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;
From:   Marijn Suijten <marijns95@gmail.com>
To:     ma.czerski@gmail.com
Cc:     linux-bluetooth@vger.kernel.org, luiz.dentz@gmail.com
Subject: Re: avrcp: possible race condition during connection establishment
Date:   Mon, 19 Oct 2020 16:16:08 +0200
Message-Id: <20201019141608.401899-1-marijns95@gmail.com>
In-Reply-To: <CAOOgLvqcSoaM7hgdzUA+JK0qgHfjhKKcLS2ScWNvsEiEj4xkoQ@mail.gmail.com>
References: <CAOOgLvqcSoaM7hgdzUA+JK0qgHfjhKKcLS2ScWNvsEiEj4xkoQ@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi Marek,

> Hi all,
>
> I was looking into, so called, absolute volume control that was
> introduced in AVRCP v1.4. What I want to achieve is to send audio from
> android smartphone to linux based device running bluez and make the
> volume control on the smartphone side to control the volume on the
> device. So the device is the a2dp sink + avrcp CT/TG and the phone is
> a2dp source + avrcp CT/TG.
>
> I assume that if all is working correctly then on the dbus the Volume
> property of the org.bluez.MediaTransport1 will be changed by the
> volume control of the phone and changes made to this property from the
> device would propagate to the phone volume slider.
>
> This is not happening and what I believe is the cause is that
> AVRCP_EVENT_VOLUME_CHANGED event registration request sent from the
> phone is rejected by the bluez. I can see that on the wireshark snoop
> from the device's bluetooth adapter. And on wireshark I see that
> AVRCP_EVENT_VOLUME_CHANGED event registration is sent by the phone
> before bluez initializes session->supported_events variable (not
> really sure about that). I think that this rejection makes the phone
> to not send SetAbsoluteVolume commands to the device on volume change.

I looked into the same issue earlier this year, see
20200118194841.439036-1-marijns95@gmail.com [1].  The gist of it is that BlueZ
bases supported_events solely on the remote AVRCP controller version, which
Android sets to 1.3 when it is a media source [2].  This version is not
relevant in your use-case because the Android phone is the AVRCP target while
blueZ is the controller.

It was decided in that mail thread to split supported_events in two; one based
on the external controller version (when BlueZ operates as target it'll
validate incoming notification registrations) and the other based on what BlueZ
currently supports as controller.

The second check might not be all too relevant and is already covered by the
switch-case; perhaps it makes more sense to base this check on the external
target version, and again validate whether we expect to receive that particular
notification registration?

Both checks together implicitly validate what BlueZ supports locally in its
role of controller or target, as remote_{target,controller}_supported_events
(anticipated names of the new members replacing supported_events) will only be
set to events that BlueZ is able to emit.

Unfortunately my ramblings in that mail shadowed an important question: how to
determine in avrcp_handle_register_notification whether BlueZ is running as
controller or target?  set_volume in transport.c derives this from
transport->source_watch but there seems to be no easy access to the
accompanying transport in avrcp_handle_register_notification.  With this
question answered I'll be able to update and resubmit the original patch.

> To test my theory i changed the session_init_control function in the
> profiles/audio/avrcp.c to call first target_init and then
> controller_init. This caused  the AVRCP_EVENT_VOLUME_CHANGED event not
> been rejected and the volume control from the phone works as expected.
>
> After reading AVRCP specification I did not find any reason for the CT
> on the phone side not to send event registration immediately after the
> AVCTP connection establishment. So I believe that bluez should not
> reject event registration in this case.
>
> Best Regards,
> Marek Czerski

Best regards,
Marijn Suijten

[1]: https://marc.info/?l=linux-bluetooth&m=157937699001093
[2]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#761