Re: [PATCH BlueZ] avrcp: keep track of last volume, and use as transport init_volume

Hi Marijn,

ti, 2021-10-12 kello 23:51 +0200, Marijn Suijten kirjoitti:

On 2021-10-12 20:35:17, Pauli Virtanen wrote:

quoted

[..]
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.

quoted

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.

quoted

[..]
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