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
