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;
Date:   Tue, 12 Oct 2021 23:51:15 +0200
From:   Marijn Suijten <marijn.suijten@somainline.org>
To:     Pauli Virtanen <pav@iki.fi>
Cc:     linux-bluetooth@vger.kernel.org,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Subject: Re: [PATCH BlueZ] avrcp: keep track of last volume, and use as
 transport init_volume
Message-ID: <20211012215115.ypzc55dd5enwscis@SoMainline.org>
References: <097b7a889f73ea9cee42b9a0c99683a1d7ee8069.camel () iki ! fi>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <097b7a889f73ea9cee42b9a0c99683a1d7ee8069.camel () iki ! fi>
Precedence: bulk

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