linux-bluetooth.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: [PATCH BlueZ] avrcp: keep track of last volume, and use as transport init_volume
       [not found] <097b7a889f73ea9cee42b9a0c99683a1d7ee8069.camel () iki ! fi>
@ 2021-10-12 21:51 ` Marijn Suijten
  2021-10-13 15:26   ` Pauli Virtanen
  0 siblings, 1 reply; 6+ messages in thread
From: Marijn Suijten @ 2021-10-12 21:51 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth, Luiz Augusto von Dentz

Hi Pauli,

On 2021-10-12 20:35:17, Pauli Virtanen wrote:
> [..]
> There's also something confusing in the code in current master
> branch: why are avrcp_handle_set_volume and avrcp_volume_changed
> setting the volume on local target->player, which I thought is a BlueZ
> as TG thing? 

If I remember correctly you can also have a player with BlueZ as CT
which is basically a passthrough representing the "actual player" on the
other end of the connection?

> I see that df7d3fa50023 ("audio/avrcp: Always update transport volume
> regardless of player") moved most of what player->set_volume does to
> the callbacks, and the player volumes seem to be currently used only to
> provide initial volume for transports. The volume probably should be
> stored elsewhere than in local players?

Going off of memory without looking at the codebase, it probably makes
sense to move volume out of the players and into the device entirely as
a single source-of-truth representing the currently known volume value
of the peer.  That might solve these issues (ie. no player available in
the linked commit) and get rid of a bunch of edgecases?  That's probably
overlooking some major dependencies though, but it'd be great if you
could look into that direction.

In addition I don't know if AVRCP volume is "local" to an entire device
or just a single transport "within" that device.

> [..]
> Caching in struct avrcp session doesn't have these problems, but
> df7d3fa50023 mentions there's some issue with avrcp session appearing
> late. I'll need to think a bit how to fix this.

I don't remember there being no avrcp session yet, but you're probably
referring to 4b6153b0501c ("audio/transport: supply volume on transport
init") instead?

- Marijn

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH BlueZ] avrcp: keep track of last volume, and use as transport init_volume
  2021-10-12 21:51 ` [PATCH BlueZ] avrcp: keep track of last volume, and use as transport init_volume Marijn Suijten
@ 2021-10-13 15:26   ` Pauli Virtanen
  2021-10-19  8:48     ` Marijn Suijten
  0 siblings, 1 reply; 6+ messages in thread
From: Pauli Virtanen @ 2021-10-13 15:26 UTC (permalink / raw)
  To: Marijn Suijten; +Cc: linux-bluetooth

Hi Marijn,

ti, 2021-10-12 kello 23:51 +0200, Marijn Suijten kirjoitti:
> 
> On 2021-10-12 20:35:17, Pauli Virtanen wrote:
> > [..]
> > There's also something confusing in the code in current master
> > branch: why are avrcp_handle_set_volume and avrcp_volume_changed
> > setting the volume on local target->player, which I thought is a BlueZ
> > as TG thing? 
> 
> If I remember correctly you can also have a player with BlueZ as CT
> which is basically a passthrough representing the "actual player" on the
> other end of the connection?

In these routines (which are called on RegisterNotification response,
and SetAbsoluteVolume response, so BlueZ as CT), the player is
from target_get_player, so it's one of the RegisterPlayer() players for
Bluez as TG.

There's indeed a separate set of "proxy" players in 
session->controller->players, but their avrcp_player_cb is NULL and
don't have set_volume defined or appear as TG players.

> > I see that df7d3fa50023 ("audio/avrcp: Always update transport volume
> > regardless of player") moved most of what player->set_volume does to
> > the callbacks, and the player volumes seem to be currently used only to
> > provide initial volume for transports. The volume probably should be
> > stored elsewhere than in local players?
> 
> Going off of memory without looking at the codebase, it probably makes
> sense to move volume out of the players and into the device entirely as
> a single source-of-truth representing the currently known volume value
> of the peer.  That might solve these issues (ie. no player available in
> the linked commit) and get rid of a bunch of edgecases?  That's probably
> overlooking some major dependencies though, but it'd be great if you
> could look into that direction.
> 
> In addition I don't know if AVRCP volume is "local" to an entire device
> or just a single transport "within" that device.

From my current reading of the spec, there's no correspondence to audio
transports. One might understand it so that volume is specific to a
certain player on TG, but as TG is supposedly sink, it's sounds like it
intended to be a single device volume.

> > [..]
> > Caching in struct avrcp session doesn't have these problems, but
> > df7d3fa50023 mentions there's some issue with avrcp session appearing
> > late. I'll need to think a bit how to fix this.
> 
> I don't remember there being no avrcp session yet, but you're probably
> referring to 4b6153b0501c ("audio/transport: supply volume on transport
> init") instead?

Sorry, yes, I meant 4b6153b0501c.

Best,
Pauli



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH BlueZ] avrcp: keep track of last volume, and use as transport init_volume
  2021-10-13 15:26   ` Pauli Virtanen
@ 2021-10-19  8:48     ` Marijn Suijten
  0 siblings, 0 replies; 6+ messages in thread
From: Marijn Suijten @ 2021-10-19  8:48 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth, Yu Liu, Sonny Sasaka

+cc'ing a bunch of editors who have previously touched Absolute Volume.

Hi Pauli,

Thanks for your patience - I wasn't available to reply last week.  Also
apologies for having the wrong In-reply-to in my mail, marc.info
substitutes @/. in email ddressses with () and ! and I missed it while
importing.  Will import directly from lore.kernel.org next time around.

On 2021-10-13 15:26:54, Pauli Virtanen wrote:
> Hi Marijn,
> 
> ti, 2021-10-12 kello 23:51 +0200, Marijn Suijten kirjoitti:
> > 
> > On 2021-10-12 20:35:17, Pauli Virtanen wrote:
> > > [..]
> > > There's also something confusing in the code in current master
> > > branch: why are avrcp_handle_set_volume and avrcp_volume_changed
> > > setting the volume on local target->player, which I thought is a BlueZ
> > > as TG thing? 
> > 
> > If I remember correctly you can also have a player with BlueZ as CT
> > which is basically a passthrough representing the "actual player" on the
> > other end of the connection?
> 
> In these routines (which are called on RegisterNotification response,
> and SetAbsoluteVolume response, so BlueZ as CT), the player is
> from target_get_player, so it's one of the RegisterPlayer() players for
> Bluez as TG.

It indeed seems like the target player is wrongly used in certain cases,
for example also when handling notifications (which should be sent from
TG->CT).  Surprisingly the last commit touching these is a crash-fix
that checks session->target for NULL, and mentions a crash exactly
because both roles might not always be supported (by the remote device,
hence the counterpart is not initialized in BlueZ).

Following your SetAbsoluteVolume response example, it also becomes clear
from avrcp_set_volume that the target should only be used in the notify
scenario - the remainder of this function uses the controller before
eventually sending the command and subsequently (seemingly wrongly)
updating the target player in the avrcp_handle_set_volume
response-handler.

However, according to 179ccb936 ("avrcp: Set volume if volume changed
event is registered") the peer doesn't need to have a target profile at
all (hence the ->controller on the BlueZ side) to control volume.
This patch seems suspicious to me and I don't even see it on the mailing
list nor GitHub to figure out if there has been any conversation prior
to merging.
Most notably registered_events should be checked when sending a
VolumeChanged notification, not when sending the SetAbsoluteVolume
command.

Perhaps the author was hit by the same versioning issue on Android where
the target profile is definitely available but advertises a version of
the spec that did not document Absolute Volume (both notification and
Set functionality).  The device however appropriately declares
FEATURE_CATEGORY_2 which mandates support for absolute volume.
I have sent a patch to support this case previously [1] but it has not
yet been looked at, Luiz should I resend this?

All in all, perhaps we should also introduce a bunch of null-checks
against ->target and ->controller respectively to ensure certain
functions are only handled for the right role?

> There's indeed a separate set of "proxy" players in 
> session->controller->players, but their avrcp_player_cb is NULL and
> don't have set_volume defined or appear as TG players.
> 
> > > I see that df7d3fa50023 ("audio/avrcp: Always update transport volume
> > > regardless of player") moved most of what player->set_volume does to
> > > the callbacks, and the player volumes seem to be currently used only to
> > > provide initial volume for transports. The volume probably should be
> > > stored elsewhere than in local players?
> > 
> > Going off of memory without looking at the codebase, it probably makes
> > sense to move volume out of the players and into the device entirely as
> > a single source-of-truth representing the currently known volume value
> > of the peer.  That might solve these issues (ie. no player available in
> > the linked commit) and get rid of a bunch of edgecases?  That's probably
> > overlooking some major dependencies though, but it'd be great if you
> > could look into that direction.
> > 
> > In addition I don't know if AVRCP volume is "local" to an entire device
> > or just a single transport "within" that device.
> 
> From my current reading of the spec, there's no correspondence to audio
> transports. One might understand it so that volume is specific to a
> certain player on TG, but as TG is supposedly sink, it's sounds like it
> intended to be a single device volume.

In that case shall we move it all outside of the players and into the
device (after maintainers acknowledge this, too)?

> > > [..]
> > > Caching in struct avrcp session doesn't have these problems, but
> > > df7d3fa50023 mentions there's some issue with avrcp session appearing
> > > late. I'll need to think a bit how to fix this.
> > 
> > I don't remember there being no avrcp session yet, but you're probably
> > referring to 4b6153b0501c ("audio/transport: supply volume on transport
> > init") instead?
> 
> Sorry, yes, I meant 4b6153b0501c.
> 
> Best,
> Pauli

- Marijn

[1]: https://marc.info/?l=linux-bluetooth&m=161911940309461&w=2

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH BlueZ] avrcp: keep track of last volume, and use as transport init_volume
  2021-10-11  4:26 ` Luiz Augusto von Dentz
@ 2021-10-12 20:35   ` Pauli Virtanen
  0 siblings, 0 replies; 6+ messages in thread
From: Pauli Virtanen @ 2021-10-12 20:35 UTC (permalink / raw)
  To: Luiz Augusto von Dentz; +Cc: linux-bluetooth

Hi Luiz,

Thanks,

su, 2021-10-10 kello 21:26 -0700, Luiz Augusto von Dentz kirjoitti:
> Hi Pauli,
> 
> On Sun, Oct 10, 2021 at 10:20 AM Pauli Virtanen <pav@iki.fi> wrote:
> > 
> > Some devices may send AVRCP VolumeChanged notification before AVDTP
> > SetConfiguration occurs, and not send another until a hardware button on
> > the device is pressed. If a media_player is registered to BlueZ, the
> > volume from the event is stored on the player, and used as init_volume
> > for new transports.  However, if no media_player is registered,
> > transports are created with volume missing.
> > 
> > If that occurs, the DBus "Volume" attribute on transports will be
> > missing until a hardware button is pressed.  Consequently, applications
> > cannot get or set volume, even though it is actually possible.
> > 
> > Address this by keeping track of the last device volume set in AVRCP
> > session. If no media_player is registered, use that as the init_volume
> > for new transports.  This has a similar effect as if a dummy media
> > player was registered.
> > 
> > This fixes AVRCP absolute volume not being available on some headphones
> > on Pipewire & Pulseaudio until HW button press.
> > ---
> >  profiles/audio/avrcp.c | 23 +++++++++++++++++++++++
> >  profiles/audio/avrcp.h |  1 +
> >  profiles/audio/media.c |  3 +++
> >  3 files changed, 27 insertions(+)
> > 
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index 7c280203c..0df416d2c 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -276,6 +276,8 @@ struct avrcp {
> >         uint8_t transaction;
> >         uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
> >         struct pending_pdu *pending_pdu;
> > +
> > +       int8_t last_device_volume;
> 
> We can probably keep this short and just call it volume.

Ok.

> >  };
> > 
> >  struct passthrough_handler {
> > @@ -1759,6 +1761,7 @@ static uint8_t avrcp_handle_set_absolute_volume(struct avrcp *session,
> >         volume = pdu->params[0] & 0x7F;
> > 
> >         media_transport_update_device_volume(session->dev, volume);
> > + session->last_device_volume = volume;
> > 
> >         return AVC_CTYPE_ACCEPTED;
> > 
> > @@ -3731,6 +3734,7 @@ static void avrcp_volume_changed(struct avrcp *session,
> > 
> >         /* Always attempt to update the transport volume */
> >         media_transport_update_device_volume(session->dev, volume);
> > + session->last_device_volume = volume;
> > 
> >         if (player)
> >                 player->cb->set_volume(volume, session->dev, player->user_data);
> > @@ -4145,6 +4149,7 @@ static void target_init(struct avrcp *session)
> > 
> >                 init_volume = media_player_get_device_volume(session->dev);
> >                 media_transport_update_device_volume(session->dev, init_volume);
> > + session->last_device_volume = init_volume;
> >         }
> > 
> >         session->supported_events |= (1 << AVRCP_EVENT_STATUS_CHANGED) |
> > @@ -4308,6 +4313,7 @@ static struct avrcp *session_create(struct avrcp_server *server,
> >         session->server = server;
> >         session->conn = avctp_connect(device);
> >         session->dev = device;
> > + session->last_device_volume = -1;
> > 
> >         server->sessions = g_slist_append(server->sessions, session);
> > 
> > @@ -4497,6 +4503,7 @@ 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);
> > + session->last_device_volume = volume;
> 
> So if I understand this right we are going to cache the volume here
> since media_transport_update_device_volume may not have a transport
> yet? If that is the case we probably need to be checking if there is a
> transport or not beforehand instead of doing this blindly.

Yes, so that avrcp volume received previously is exhibited in
transports created next. I now see that this is the same issue as in
4b6153b0501c ("audio/transport: supply volume on transport init"), but
in the case when there are no local players registered, and the fix
there does not work.

Currently, if there is a local player registered, the current volume is
cached in the player volume whether there are transports or not.
Caching always in struct avrcp would then make the zero-players case
the same. I don't immediately see something incorrect.

Albeit, this probably should be only done for BlueZ as CT, so shouldn't
cache in avrcp_handle_set_absolute_volume. That could be a minimal v2?

***

There's also something confusing in the code in current master
branch: why are avrcp_handle_set_volume and avrcp_volume_changed
setting the volume on local target->player, which I thought is a BlueZ
as TG thing? 

I see that df7d3fa50023 ("audio/avrcp: Always update transport volume
regardless of player") moved most of what player->set_volume does to
the callbacks, and the player volumes seem to be currently used only to
provide initial volume for transports. The volume probably should be
stored elsewhere than in local players?

In fact, it causes problems because there's just a single player that
has long lifetime: BlueZ sink/TG use as the initial volume, the volume
of a previously connected different remote device sink/TG. Also, having
multiple remote device TG/sink makes the initial volumes conflict. So
there seems to be something to fix (separate patch?).

Caching in struct avrcp session doesn't have these problems, but
df7d3fa50023 mentions there's some issue with avrcp session appearing
late. I'll need to think a bit how to fix this.

> > 
> >         if (player != NULL)
> >                 player->cb->set_volume(volume, session->dev, player->user_data);
> > @@ -4598,6 +4605,22 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
> >                                         avrcp_handle_set_volume, session);
> >  }
> > 
> > +int8_t avrcp_get_last_volume(struct btd_device *dev)
> > +{
> > +       struct avrcp_server *server;
> > +       struct avrcp *session;
> > +
> > +       server = find_server(servers, device_get_adapter(dev));
> > +       if (server == NULL)
> > +               return -1;
> > +
> > +       session = find_session(server->sessions, dev);
> > +       if (session == NULL)
> > +               return -1;
> > +
> > +       return session->last_device_volume;
> > +}
> > +
> >  struct avrcp_player *avrcp_get_target_player_by_device(struct btd_device *dev)
> >  {
> >         struct avrcp_server *server;
> > diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
> > index dcc580e37..952f0eea9 100644
> > --- a/profiles/audio/avrcp.h
> > +++ b/profiles/audio/avrcp.h
> > @@ -91,6 +91,7 @@ struct avrcp_player_cb {
> >  };
> > 
> >  int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify);
> > +int8_t avrcp_get_last_volume(struct btd_device *dev);
> 
> Let's have it as avrcp_get_volume so it is symmetric to avrcp_set_volume.

Ok.

> >  struct avrcp_player *avrcp_register_player(struct btd_adapter
> > *adapter,
> >                                                 struct
> > avrcp_player_cb *cb,
> > diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> > index 521902ed8..a37378393 100644
> > --- a/profiles/audio/media.c
> > +++ b/profiles/audio/media.c
> > @@ -494,6 +494,9 @@ static gboolean set_configuration(struct
> > media_endpoint *endpoint,
> >                 return FALSE;
> > 
> >         init_volume = media_player_get_device_volume(device);
> > + if (init_volume < 0)
> > + init_volume = avrcp_get_last_volume(device);
> 
> I wonder if we shouldn't be better to move the call to
> avrcp_get_volume inside media_player_get_device_volume so it does the
> fallback automatically if there is no volume set.

Ok.

> >         media_transport_update_volume(transport, init_volume);
> > 
> >         msg = dbus_message_new_method_call(endpoint->sender,
> > endpoint->path,
> > --
> > 2.31.1
> > 
> 
> 






^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH BlueZ] avrcp: keep track of last volume, and use as transport init_volume
  2021-10-10 17:14 Pauli Virtanen
@ 2021-10-11  4:26 ` Luiz Augusto von Dentz
  2021-10-12 20:35   ` Pauli Virtanen
  0 siblings, 1 reply; 6+ messages in thread
From: Luiz Augusto von Dentz @ 2021-10-11  4:26 UTC (permalink / raw)
  To: Pauli Virtanen; +Cc: linux-bluetooth

Hi Pauli,

On Sun, Oct 10, 2021 at 10:20 AM Pauli Virtanen <pav@iki.fi> wrote:
>
> Some devices may send AVRCP VolumeChanged notification before AVDTP
> SetConfiguration occurs, and not send another until a hardware button on
> the device is pressed. If a media_player is registered to BlueZ, the
> volume from the event is stored on the player, and used as init_volume
> for new transports.  However, if no media_player is registered,
> transports are created with volume missing.
>
> If that occurs, the DBus "Volume" attribute on transports will be
> missing until a hardware button is pressed.  Consequently, applications
> cannot get or set volume, even though it is actually possible.
>
> Address this by keeping track of the last device volume set in AVRCP
> session. If no media_player is registered, use that as the init_volume
> for new transports.  This has a similar effect as if a dummy media
> player was registered.
>
> This fixes AVRCP absolute volume not being available on some headphones
> on Pipewire & Pulseaudio until HW button press.
> ---
>  profiles/audio/avrcp.c | 23 +++++++++++++++++++++++
>  profiles/audio/avrcp.h |  1 +
>  profiles/audio/media.c |  3 +++
>  3 files changed, 27 insertions(+)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index 7c280203c..0df416d2c 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -276,6 +276,8 @@ struct avrcp {
>         uint8_t transaction;
>         uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
>         struct pending_pdu *pending_pdu;
> +
> +       int8_t last_device_volume;

We can probably keep this short and just call it volume.

>  };
>
>  struct passthrough_handler {
> @@ -1759,6 +1761,7 @@ static uint8_t avrcp_handle_set_absolute_volume(struct avrcp *session,
>         volume = pdu->params[0] & 0x7F;
>
>         media_transport_update_device_volume(session->dev, volume);
> +       session->last_device_volume = volume;
>
>         return AVC_CTYPE_ACCEPTED;
>
> @@ -3731,6 +3734,7 @@ static void avrcp_volume_changed(struct avrcp *session,
>
>         /* Always attempt to update the transport volume */
>         media_transport_update_device_volume(session->dev, volume);
> +       session->last_device_volume = volume;
>
>         if (player)
>                 player->cb->set_volume(volume, session->dev, player->user_data);
> @@ -4145,6 +4149,7 @@ static void target_init(struct avrcp *session)
>
>                 init_volume = media_player_get_device_volume(session->dev);
>                 media_transport_update_device_volume(session->dev, init_volume);
> +               session->last_device_volume = init_volume;
>         }
>
>         session->supported_events |= (1 << AVRCP_EVENT_STATUS_CHANGED) |
> @@ -4308,6 +4313,7 @@ static struct avrcp *session_create(struct avrcp_server *server,
>         session->server = server;
>         session->conn = avctp_connect(device);
>         session->dev = device;
> +       session->last_device_volume = -1;
>
>         server->sessions = g_slist_append(server->sessions, session);
>
> @@ -4497,6 +4503,7 @@ 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);
> +       session->last_device_volume = volume;

So if I understand this right we are going to cache the volume here
since media_transport_update_device_volume may not have a transport
yet? If that is the case we probably need to be checking if there is a
transport or not beforehand instead of doing this blindly.

>
>         if (player != NULL)
>                 player->cb->set_volume(volume, session->dev, player->user_data);
> @@ -4598,6 +4605,22 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
>                                         avrcp_handle_set_volume, session);
>  }
>
> +int8_t avrcp_get_last_volume(struct btd_device *dev)
> +{
> +       struct avrcp_server *server;
> +       struct avrcp *session;
> +
> +       server = find_server(servers, device_get_adapter(dev));
> +       if (server == NULL)
> +               return -1;
> +
> +       session = find_session(server->sessions, dev);
> +       if (session == NULL)
> +               return -1;
> +
> +       return session->last_device_volume;
> +}
> +
>  struct avrcp_player *avrcp_get_target_player_by_device(struct btd_device *dev)
>  {
>         struct avrcp_server *server;
> diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
> index dcc580e37..952f0eea9 100644
> --- a/profiles/audio/avrcp.h
> +++ b/profiles/audio/avrcp.h
> @@ -91,6 +91,7 @@ struct avrcp_player_cb {
>  };
>
>  int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify);
> +int8_t avrcp_get_last_volume(struct btd_device *dev);

Let's have it as avrcp_get_volume so it is symmetric to avrcp_set_volume.

>  struct avrcp_player *avrcp_register_player(struct btd_adapter *adapter,
>                                                 struct avrcp_player_cb *cb,
> diff --git a/profiles/audio/media.c b/profiles/audio/media.c
> index 521902ed8..a37378393 100644
> --- a/profiles/audio/media.c
> +++ b/profiles/audio/media.c
> @@ -494,6 +494,9 @@ static gboolean set_configuration(struct media_endpoint *endpoint,
>                 return FALSE;
>
>         init_volume = media_player_get_device_volume(device);
> +       if (init_volume < 0)
> +               init_volume = avrcp_get_last_volume(device);

I wonder if we shouldn't be better to move the call to
avrcp_get_volume inside media_player_get_device_volume so it does the
fallback automatically if there is no volume set.

>         media_transport_update_volume(transport, init_volume);
>
>         msg = dbus_message_new_method_call(endpoint->sender, endpoint->path,
> --
> 2.31.1
>


-- 
Luiz Augusto von Dentz

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH BlueZ] avrcp: keep track of last volume, and use as transport init_volume
@ 2021-10-10 17:14 Pauli Virtanen
  2021-10-11  4:26 ` Luiz Augusto von Dentz
  0 siblings, 1 reply; 6+ messages in thread
From: Pauli Virtanen @ 2021-10-10 17:14 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Pauli Virtanen

Some devices may send AVRCP VolumeChanged notification before AVDTP
SetConfiguration occurs, and not send another until a hardware button on
the device is pressed. If a media_player is registered to BlueZ, the
volume from the event is stored on the player, and used as init_volume
for new transports.  However, if no media_player is registered,
transports are created with volume missing.

If that occurs, the DBus "Volume" attribute on transports will be
missing until a hardware button is pressed.  Consequently, applications
cannot get or set volume, even though it is actually possible.

Address this by keeping track of the last device volume set in AVRCP
session. If no media_player is registered, use that as the init_volume
for new transports.  This has a similar effect as if a dummy media
player was registered.

This fixes AVRCP absolute volume not being available on some headphones
on Pipewire & Pulseaudio until HW button press.
---
 profiles/audio/avrcp.c | 23 +++++++++++++++++++++++
 profiles/audio/avrcp.h |  1 +
 profiles/audio/media.c |  3 +++
 3 files changed, 27 insertions(+)

diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index 7c280203c..0df416d2c 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -276,6 +276,8 @@ struct avrcp {
 	uint8_t transaction;
 	uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
 	struct pending_pdu *pending_pdu;
+
+	int8_t last_device_volume;
 };
 
 struct passthrough_handler {
@@ -1759,6 +1761,7 @@ static uint8_t avrcp_handle_set_absolute_volume(struct avrcp *session,
 	volume = pdu->params[0] & 0x7F;
 
 	media_transport_update_device_volume(session->dev, volume);
+	session->last_device_volume = volume;
 
 	return AVC_CTYPE_ACCEPTED;
 
@@ -3731,6 +3734,7 @@ static void avrcp_volume_changed(struct avrcp *session,
 
 	/* Always attempt to update the transport volume */
 	media_transport_update_device_volume(session->dev, volume);
+	session->last_device_volume = volume;
 
 	if (player)
 		player->cb->set_volume(volume, session->dev, player->user_data);
@@ -4145,6 +4149,7 @@ static void target_init(struct avrcp *session)
 
 		init_volume = media_player_get_device_volume(session->dev);
 		media_transport_update_device_volume(session->dev, init_volume);
+		session->last_device_volume = init_volume;
 	}
 
 	session->supported_events |= (1 << AVRCP_EVENT_STATUS_CHANGED) |
@@ -4308,6 +4313,7 @@ static struct avrcp *session_create(struct avrcp_server *server,
 	session->server = server;
 	session->conn = avctp_connect(device);
 	session->dev = device;
+	session->last_device_volume = -1;
 
 	server->sessions = g_slist_append(server->sessions, session);
 
@@ -4497,6 +4503,7 @@ 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);
+	session->last_device_volume = volume;
 
 	if (player != NULL)
 		player->cb->set_volume(volume, session->dev, player->user_data);
@@ -4598,6 +4605,22 @@ int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify)
 					avrcp_handle_set_volume, session);
 }
 
+int8_t avrcp_get_last_volume(struct btd_device *dev)
+{
+	struct avrcp_server *server;
+	struct avrcp *session;
+
+	server = find_server(servers, device_get_adapter(dev));
+	if (server == NULL)
+		return -1;
+
+	session = find_session(server->sessions, dev);
+	if (session == NULL)
+		return -1;
+
+	return session->last_device_volume;
+}
+
 struct avrcp_player *avrcp_get_target_player_by_device(struct btd_device *dev)
 {
 	struct avrcp_server *server;
diff --git a/profiles/audio/avrcp.h b/profiles/audio/avrcp.h
index dcc580e37..952f0eea9 100644
--- a/profiles/audio/avrcp.h
+++ b/profiles/audio/avrcp.h
@@ -91,6 +91,7 @@ struct avrcp_player_cb {
 };
 
 int avrcp_set_volume(struct btd_device *dev, int8_t volume, bool notify);
+int8_t avrcp_get_last_volume(struct btd_device *dev);
 
 struct avrcp_player *avrcp_register_player(struct btd_adapter *adapter,
 						struct avrcp_player_cb *cb,
diff --git a/profiles/audio/media.c b/profiles/audio/media.c
index 521902ed8..a37378393 100644
--- a/profiles/audio/media.c
+++ b/profiles/audio/media.c
@@ -494,6 +494,9 @@ static gboolean set_configuration(struct media_endpoint *endpoint,
 		return FALSE;
 
 	init_volume = media_player_get_device_volume(device);
+	if (init_volume < 0)
+		init_volume = avrcp_get_last_volume(device);
+
 	media_transport_update_volume(transport, init_volume);
 
 	msg = dbus_message_new_method_call(endpoint->sender, endpoint->path,
-- 
2.31.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-10-19  8:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <097b7a889f73ea9cee42b9a0c99683a1d7ee8069.camel () iki ! fi>
2021-10-12 21:51 ` [PATCH BlueZ] avrcp: keep track of last volume, and use as transport init_volume Marijn Suijten
2021-10-13 15:26   ` Pauli Virtanen
2021-10-19  8:48     ` Marijn Suijten
2021-10-10 17:14 Pauli Virtanen
2021-10-11  4:26 ` Luiz Augusto von Dentz
2021-10-12 20:35   ` Pauli Virtanen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).