Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Message-ID: <2a183e606205246ae64422c274275eb1caa2ada5.camel@iki.fi>
Subject: Re: [PATCH BlueZ] avrcp: keep track of last volume, and use as
 transport init_volume
From:   Pauli Virtanen <pav@iki.fi>
To:     Marijn Suijten <marijn.suijten@somainline.org>
Cc:     linux-bluetooth@vger.kernel.org
Date:   Wed, 13 Oct 2021 15:26:54 +0000
In-Reply-To: <20211012215115.ypzc55dd5enwscis@SoMainline.org>
References: <097b7a889f73ea9cee42b9a0c99683a1d7ee8069.camel () iki ! fi>
         <20211012215115.ypzc55dd5enwscis@SoMainline.org>
Content-Type: text/plain; charset="UTF-8"
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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