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.
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
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
[email protected] [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
Hi Marijn,
pon., 19 paź 2020 o 16:16 Marijn Suijten <[email protected]> napisał(a):
>
> 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
> [email protected] [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.
>
I didn't tested Your patch but after looking at the code it seems that
just applying Your patch would solve my problem.
Regarding avrcp version, in android there is developer option to set
avrcp version. For example my Xiaomi redmi 8 (android 10) reports
version according to this setting, but samsung galaxy s7 (android 8)
always report version 1.4 regardless of this setting.
> 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.
>
One thing is not clear for me, what is the purpose of the
supported_events ? It is used in two places:
First is the avrcp_handle_register_notification function. If the
remote side want to register itself for specific event notification it
does not matter what version of avrcp that remote side supports. If it
ask for specific event it clearly support that event.
Second is in avrcp_handle_get_capabilities in CAP_EVENTS_SUPPORTED
case. Does it matter if local side reply with events that are not
supported in the version of avrcp supported by the remote side ?
> 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
Best regards,
Marek Czerski
Hi Marek, Marijn,
On Mon, Oct 19, 2020 at 9:06 AM Marek Czerski <[email protected]> wrote:
>
> Hi Marijn,
>
> pon., 19 paź 2020 o 16:16 Marijn Suijten <[email protected]> napisał(a):
> >
> > 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
> > [email protected] [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.
> >
>
> I didn't tested Your patch but after looking at the code it seems that
> just applying Your patch would solve my problem.
> Regarding avrcp version, in android there is developer option to set
> avrcp version. For example my Xiaomi redmi 8 (android 10) reports
> version according to this setting, but samsung galaxy s7 (android 8)
> always report version 1.4 regardless of this setting.
We need to rework a little bit how the controller/target works, this
roles are actually supposed to be interpreted as client/server and
much like GATT they can be used simultaneously, so we need a target
versions and a controller version and independent supported events.
That said if the remote side controller version does not indicate 1.4
or later we obviously can't support absolute volume control as that is
reserved in earlier versions.
> > 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.
> >
>
> One thing is not clear for me, what is the purpose of the
> supported_events ? It is used in two places:
> First is the avrcp_handle_register_notification function. If the
> remote side want to register itself for specific event notification it
> does not matter what version of avrcp that remote side supports. If it
> ask for specific event it clearly support that event.
> Second is in avrcp_handle_get_capabilities in CAP_EVENTS_SUPPORTED
> case. Does it matter if local side reply with events that are not
> supported in the version of avrcp supported by the remote side ?
As I said we need to split the supported events to
ct(client)/tg(server) to avoid interpreting them as the same, it is
very odd that the remote would have different versions for each role
but it looks like this is happening in this case although it is work
confirming if the CT version if in fact 1.3 as well we cannot enable
absolute volume control as that is not supported by that version, what
we can perhaps is to detect if SetAbsoluteVolume control is used then
update the events for the session.
> > 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
>
>
> Best regards,
> Marek Czerski
--
Luiz Augusto von Dentz
Hi Luiz, Marek,
On Mon, 19 Oct 2020 at 18:47, Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Marek, Marijn,
>
> On Mon, Oct 19, 2020 at 9:06 AM Marek Czerski <[email protected]> wrote:
> >
> > Hi Marijn,
> >
> > pon., 19 paź 2020 o 16:16 Marijn Suijten <[email protected]> napisał(a):
> > >
> > > 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
> > > [email protected] [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.
> > >
> >
> > I didn't tested Your patch but after looking at the code it seems that
> > just applying Your patch would solve my problem.
It should, even though I don't immediately see how swapping
target/controller_init as per your initial email solves the problem.
> > Regarding avrcp version, in android there is developer option to set
> > avrcp version. For example my Xiaomi redmi 8 (android 10) reports
> > version according to this setting, but samsung galaxy s7 (android 8)
> > always report version 1.4 regardless of this setting.
I think this is the version used for the AVRCP Remote Target [1]. The
version linked above, the one that is causing problems here is that of
the AVRCP controller.
There have always been problems changing these settings in developer
options - sometimes they work, sometimes they don't. The system has
been overhauled for Android 11 and is now much more consistent though
avrcp still runs through a property. If I remember correctly
restarting bluetooth or re-adding a device (in case of SDP caching) is
the only solution.
> We need to rework a little bit how the controller/target works, this
> roles are actually supposed to be interpreted as client/server and
> much like GATT they can be used simultaneously, so we need a target
> versions and a controller version and independent supported events.
Yes, I think that's the change we proposed last time and are proposing
now. Unfortunately relevant functions need to know if the local
(BlueZ) end is operating as target or controller for that particular
operation.
> That said if the remote side controller version does not indicate 1.4
> or later we obviously can't support absolute volume control as that is
> reserved in earlier versions.
That depends on the way audio is flowing. When the local (BlueZ) end
is the sink (rendering the audio) meaning it takes the role of AVRCP
controller (the situation in the mail from Marek), the remote must be
the AVRCP target and hence its controller version is irrelevant.
Likewise when BlueZ is the audio source (AVRCP target), it should only
care about the remote controller profile and version.
But that is exactly what you described above: BlueZ needs to attain
separation between handling notification registrations (and perhaps
other code) as a controller and target separately.
> > > 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.
> > >
> >
> > One thing is not clear for me, what is the purpose of the
> > supported_events ? It is used in two places:
> > First is the avrcp_handle_register_notification function. If the
> > remote side want to register itself for specific event notification it
> > does not matter what version of avrcp that remote side supports. If it
> > ask for specific event it clearly support that event.
Looking at commit 4ae6135 that introduced this check it was apparently
causing crashes without. I can only guess that devices were sending
vendor-specific notification requests, assuming implementations are
supposed to filter out anything beyond what their reported version
supports? Either way input validation is still a sane thing to do.
> > Second is in avrcp_handle_get_capabilities in CAP_EVENTS_SUPPORTED
> > case. Does it matter if local side reply with events that are not
> > supported in the version of avrcp supported by the remote side ?
I guess it is sane here to not report capabilities the CT should -
given it's version - not know about. I could not find anything in the
1.6.2 spec mandating this though, perhaps Luiz knows.
> As I said we need to split the supported events to
> ct(client)/tg(server) to avoid interpreting them as the same, it is
> very odd that the remote would have different versions for each role
Yeah, Android exposes different versions for its AVRCP Remote and
AVRCP Remote Target.
This is slightly confusing though: we discussed earlier that CT and TG
is about the initiator and receiver respectively, with no direct
mapping to AVRCP remote and target as either side can be the initiator
(sender) of a command. Correct?
In this specific case a notification registration will always be
initiated by a CT and fulfilled by the TG, no matter whether it is an
AVRCP remote or target.
> but it looks like this is happening in this case although it is work
> confirming if the CT version if in fact 1.3 as well we cannot enable
> absolute volume control as that is not supported by that version, what
> we can perhaps is to detect if SetAbsoluteVolume control is used then
> update the events for the session.
Again, in the specific case of this issue it is fine if the CT
(initiator, the Android phone, supposed to behave like the target as
it is the audio source) reports a controller version of 1.3 as we are
only concerned about the target version, which will be the one
registering for EVENT_VOLUME_CHANGED notifications from the
sink/controller (BlueZ). Android always reports the AVRCP Remote
Target as 1.4 or higher based on developer settings [1].
> > > 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
> >
> >
> > Best regards,
> > Marek Czerski
>
>
>
> --
> Luiz Augusto von Dentz
P.S.: I hope it is fine to respond to two emails in one, seems like
that will get confusing real quick if depth increases.
- Marijn
[1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#612
Hi Marijn,
pon., 19 paź 2020 o 21:53 Marijn Suijten <[email protected]> napisał(a):
>
> Hi Luiz, Marek,
>
> On Mon, 19 Oct 2020 at 18:47, Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Marek, Marijn,
> >
> > On Mon, Oct 19, 2020 at 9:06 AM Marek Czerski <[email protected]> wrote:
> > >
> > > Hi Marijn,
> > >
> > > pon., 19 paź 2020 o 16:16 Marijn Suijten <[email protected]> napisał(a):
> > > >
> > > > 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
> > > > [email protected] [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.
> > > >
> > >
> > > I didn't tested Your patch but after looking at the code it seems that
> > > just applying Your patch would solve my problem.
>
> It should, even though I don't immediately see how swapping
> target/controller_init as per your initial email solves the problem.
>
The sequence of events is important here. The
session->supported_events must be initialised (which is done in
target_init) before first AVRCP_REGISTER_NOTIFICATION command from the
remote side controller. If supported_events is not initialised and
AVRCP_REGISTER_NOTIFICATION arrives the
avrcp_handle_register_notification function rejects the event
registration. And this is what I see in wireshark.
And in my case, calling target_init earlier (the is before
controller_init) solves the problem because supported_events is
already initialised at the time the AVRCP_REGISTER_NOTIFICATION
arrives.
But I think that the problem is more general. There is some gap
between AVCTP connection establishment and BlueZ readiness to handle
commands from the remote side. There could be devices that send
commands during this gap and those devices would not work correctly
with BlueZ despite the fact that they are consistent with the
specification (or not ?).
> > > Regarding avrcp version, in android there is developer option to set
> > > avrcp version. For example my Xiaomi redmi 8 (android 10) reports
> > > version according to this setting, but samsung galaxy s7 (android 8)
> > > always report version 1.4 regardless of this setting.
>
> I think this is the version used for the AVRCP Remote Target [1]. The
> version linked above, the one that is causing problems here is that of
> the AVRCP controller.
>
> There have always been problems changing these settings in developer
> options - sometimes they work, sometimes they don't. The system has
> been overhauled for Android 11 and is now much more consistent though
> avrcp still runs through a property. If I remember correctly
> restarting bluetooth or re-adding a device (in case of SDP caching) is
> the only solution.
>
> > We need to rework a little bit how the controller/target works, this
> > roles are actually supposed to be interpreted as client/server and
> > much like GATT they can be used simultaneously, so we need a target
> > versions and a controller version and independent supported events.
>
> Yes, I think that's the change we proposed last time and are proposing
> now. Unfortunately relevant functions need to know if the local
> (BlueZ) end is operating as target or controller for that particular
> operation.
>
> > That said if the remote side controller version does not indicate 1.4
> > or later we obviously can't support absolute volume control as that is
> > reserved in earlier versions.
>
> That depends on the way audio is flowing. When the local (BlueZ) end
> is the sink (rendering the audio) meaning it takes the role of AVRCP
> controller (the situation in the mail from Marek), the remote must be
> the AVRCP target and hence its controller version is irrelevant.
> Likewise when BlueZ is the audio source (AVRCP target), it should only
> care about the remote controller profile and version.
> But that is exactly what you described above: BlueZ needs to attain
> separation between handling notification registrations (and perhaps
> other code) as a controller and target separately.
>
I don't know if I'm interpreting your words correctly but I think that
You make an assumption that a2dp sink must be avrcp CT and a2dp source
is avrcp TG.
In case of the absolute volume control it is not true. For absolute
volume control to work it is the other way around which makes both
sides be CT and TG at the same time. a2dp sink is avrcp CT for
controlling playback and a2dp source is avrcp CT for setting the
volume. It is the a2dp source that is sending
AVRCP_SET_ABSOLUTE_VOLUME commands.
> > > > 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.
> > > >
> > >
> > > One thing is not clear for me, what is the purpose of the
> > > supported_events ? It is used in two places:
> > > First is the avrcp_handle_register_notification function. If the
> > > remote side want to register itself for specific event notification it
> > > does not matter what version of avrcp that remote side supports. If it
> > > ask for specific event it clearly support that event.
>
> Looking at commit 4ae6135 that introduced this check it was apparently
> causing crashes without. I can only guess that devices were sending
> vendor-specific notification requests, assuming implementations are
> supposed to filter out anything beyond what their reported version
> supports? Either way input validation is still a sane thing to do.
>
> > > Second is in avrcp_handle_get_capabilities in CAP_EVENTS_SUPPORTED
> > > case. Does it matter if local side reply with events that are not
> > > supported in the version of avrcp supported by the remote side ?
>
> I guess it is sane here to not report capabilities the CT should -
> given it's version - not know about. I could not find anything in the
> 1.6.2 spec mandating this though, perhaps Luiz knows.
>
> > As I said we need to split the supported events to
> > ct(client)/tg(server) to avoid interpreting them as the same, it is
> > very odd that the remote would have different versions for each role
>
> Yeah, Android exposes different versions for its AVRCP Remote and
> AVRCP Remote Target.
> This is slightly confusing though: we discussed earlier that CT and TG
> is about the initiator and receiver respectively, with no direct
> mapping to AVRCP remote and target as either side can be the initiator
> (sender) of a command. Correct?
> In this specific case a notification registration will always be
> initiated by a CT and fulfilled by the TG, no matter whether it is an
> AVRCP remote or target.
>
> > but it looks like this is happening in this case although it is work
> > confirming if the CT version if in fact 1.3 as well we cannot enable
> > absolute volume control as that is not supported by that version, what
> > we can perhaps is to detect if SetAbsoluteVolume control is used then
> > update the events for the session.
>
> Again, in the specific case of this issue it is fine if the CT
> (initiator, the Android phone, supposed to behave like the target as
> it is the audio source) reports a controller version of 1.3 as we are
> only concerned about the target version, which will be the one
> registering for EVENT_VOLUME_CHANGED notifications from the
> sink/controller (BlueZ). Android always reports the AVRCP Remote
> Target as 1.4 or higher based on developer settings [1].
>
> > > > 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
> > >
> > >
> > > Best regards,
> > > Marek Czerski
> >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> P.S.: I hope it is fine to respond to two emails in one, seems like
> that will get confusing real quick if depth increases.
>
> - Marijn
>
> [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#612
Best regards,
Marek
Hi Marek,
(Apologies for sending this to you twice; missed the reply-all button
so this didn't end up on the mailing list)
On Tue, 20 Oct 2020 at 09:31, Marek Czerski <[email protected]> wrote:
>
> Hi Marijn,
>
> pon., 19 paź 2020 o 21:53 Marijn Suijten <[email protected]> napisał(a):
> >
> > Hi Luiz, Marek,
> >
> > On Mon, 19 Oct 2020 at 18:47, Luiz Augusto von Dentz
> > <[email protected]> wrote:
> > >
> > > Hi Marek, Marijn,
> > >
> > > On Mon, Oct 19, 2020 at 9:06 AM Marek Czerski <[email protected]> wrote:
> > > >
> > > > Hi Marijn,
> > > >
> > > > pon., 19 paź 2020 o 16:16 Marijn Suijten <[email protected]> napisał(a):
> > > > >
> > > > > 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
> > > > > [email protected] [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.
> > > > >
> > > >
> > > > I didn't tested Your patch but after looking at the code it seems that
> > > > just applying Your patch would solve my problem.
> >
> > It should, even though I don't immediately see how swapping
> > target/controller_init as per your initial email solves the problem.
> >
>
> The sequence of events is important here. The
> session->supported_events must be initialised (which is done in
> target_init) before first AVRCP_REGISTER_NOTIFICATION command from the
> remote side controller. If supported_events is not initialised and
> AVRCP_REGISTER_NOTIFICATION arrives the
> avrcp_handle_register_notification function rejects the event
> registration. And this is what I see in wireshark.
That's interesting and means we are dealing with two distinct problems
here. No matter what order I flip controller/target_init in
AVRCP_REGISTER_NOTIFICATION is always received after both functions
complete. handle_vendordep_pdu is registered before these
initialization functions making this possible though. I wonder if PDUs
are dropped when the callback is registered later, after init. Perhaps
controller/target_init need to be split in two, a static
initialization part based on the remote profile version followed by
registering this callback and finally functions that send commands to
the remote like avrcp_get_capabilities.
> And in my case, calling target_init earlier (the is before
> controller_init) solves the problem because supported_events is
> already initialised at the time the AVRCP_REGISTER_NOTIFICATION
> arrives.
That means your phone reports an AVRCP remote version of at least
0x104, otherwise supported_events would not contain
AVRCP_EVENT_VOLUME_CHANGED. Can you validate this in wireshark or
DBG("%p version 0x%04x", ...) in the logs? I'm seeing:
profiles/audio/avrcp.c:target_init() 0x5614a2cbab70 version 0x0103
Either way the split Luiz and I mentioned mean one variant of
supported_events (the one relevant for registering
AVRCP_EVENT_VOLUME_CHANGED) is initialized in controller_init.
> But I think that the problem is more general. There is some gap
> between AVCTP connection establishment and BlueZ readiness to handle
> commands from the remote side. There could be devices that send
> commands during this gap and those devices would not work correctly
> with BlueZ despite the fact that they are consistent with the
> specification (or not ?).
>
> > > > Regarding avrcp version, in android there is developer option to set
> > > > avrcp version. For example my Xiaomi redmi 8 (android 10) reports
> > > > version according to this setting, but samsung galaxy s7 (android 8)
> > > > always report version 1.4 regardless of this setting.
> >
> > I think this is the version used for the AVRCP Remote Target [1]. The
> > version linked above, the one that is causing problems here is that of
> > the AVRCP controller.
> >
> > There have always been problems changing these settings in developer
> > options - sometimes they work, sometimes they don't. The system has
> > been overhauled for Android 11 and is now much more consistent though
> > avrcp still runs through a property. If I remember correctly
> > restarting bluetooth or re-adding a device (in case of SDP caching) is
> > the only solution.
> >
> > > We need to rework a little bit how the controller/target works, this
> > > roles are actually supposed to be interpreted as client/server and
> > > much like GATT they can be used simultaneously, so we need a target
> > > versions and a controller version and independent supported events.
> >
> > Yes, I think that's the change we proposed last time and are proposing
> > now. Unfortunately relevant functions need to know if the local
> > (BlueZ) end is operating as target or controller for that particular
> > operation.
> >
> > > That said if the remote side controller version does not indicate 1.4
> > > or later we obviously can't support absolute volume control as that is
> > > reserved in earlier versions.
> >
> > That depends on the way audio is flowing. When the local (BlueZ) end
> > is the sink (rendering the audio) meaning it takes the role of AVRCP
> > controller (the situation in the mail from Marek), the remote must be
> > the AVRCP target and hence its controller version is irrelevant.
> > Likewise when BlueZ is the audio source (AVRCP target), it should only
> > care about the remote controller profile and version.
> > But that is exactly what you described above: BlueZ needs to attain
> > separation between handling notification registrations (and perhaps
> > other code) as a controller and target separately.
> >
>
> I don't know if I'm interpreting your words correctly but I think that
> You make an assumption that a2dp sink must be avrcp CT and a2dp source
> is avrcp TG.
> In case of the absolute volume control it is not true. For absolute
> volume control to work it is the other way around which makes both
> sides be CT and TG at the same time.
Quite the contrary, in my previous mail I attempted to explain
multiple times that the CT/TG designation in the spec is used for
initiator and target respectively, not the AVRCP controller and target
profile. Indeed, depending on the transaction either side (controller
or target) can be the initiator (CT). In other words CT and TG are
_not_ abbreviations for the controller and target profile.
> a2dp sink is avrcp CT for
> controlling playback and a2dp source is avrcp CT for setting the
> volume. It is the a2dp source that is sending
> AVRCP_SET_ABSOLUTE_VOLUME commands.
Correct, yet the A2DP sink will always take on the AVRCP controller
profile where the phone being the A2DP source takes the AVRCP target
profile.
> > > > > 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.
> > > > >
> > > >
> > > > One thing is not clear for me, what is the purpose of the
> > > > supported_events ? It is used in two places:
> > > > First is the avrcp_handle_register_notification function. If the
> > > > remote side want to register itself for specific event notification it
> > > > does not matter what version of avrcp that remote side supports. If it
> > > > ask for specific event it clearly support that event.
> >
> > Looking at commit 4ae6135 that introduced this check it was apparently
> > causing crashes without. I can only guess that devices were sending
> > vendor-specific notification requests, assuming implementations are
> > supposed to filter out anything beyond what their reported version
> > supports? Either way input validation is still a sane thing to do.
> >
> > > > Second is in avrcp_handle_get_capabilities in CAP_EVENTS_SUPPORTED
> > > > case. Does it matter if local side reply with events that are not
> > > > supported in the version of avrcp supported by the remote side ?
> >
> > I guess it is sane here to not report capabilities the CT should -
> > given it's version - not know about. I could not find anything in the
> > 1.6.2 spec mandating this though, perhaps Luiz knows.
> >
> > > As I said we need to split the supported events to
> > > ct(client)/tg(server) to avoid interpreting them as the same, it is
> > > very odd that the remote would have different versions for each role
> >
> > Yeah, Android exposes different versions for its AVRCP Remote and
> > AVRCP Remote Target.
> > This is slightly confusing though: we discussed earlier that CT and TG
> > is about the initiator and receiver respectively, with no direct
> > mapping to AVRCP remote and target as either side can be the initiator
> > (sender) of a command. Correct?
> > In this specific case a notification registration will always be
> > initiated by a CT and fulfilled by the TG, no matter whether it is an
> > AVRCP remote or target.
> >
> > > but it looks like this is happening in this case although it is work
> > > confirming if the CT version if in fact 1.3 as well we cannot enable
> > > absolute volume control as that is not supported by that version, what
> > > we can perhaps is to detect if SetAbsoluteVolume control is used then
> > > update the events for the session.
> >
> > Again, in the specific case of this issue it is fine if the CT
> > (initiator, the Android phone, supposed to behave like the target as
> > it is the audio source) reports a controller version of 1.3 as we are
> > only concerned about the target version, which will be the one
> > registering for EVENT_VOLUME_CHANGED notifications from the
> > sink/controller (BlueZ). Android always reports the AVRCP Remote
> > Target as 1.4 or higher based on developer settings [1].
> >
> > > > > 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
> > > >
> > > >
> > > > Best regards,
> > > > Marek Czerski
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > P.S.: I hope it is fine to respond to two emails in one, seems like
> > that will get confusing real quick if depth increases.
> >
> > - Marijn
> >
> > [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#612
>
> Best regards,
> Marek
- Marijn
Hi Marijn,
wt., 20 paź 2020 o 11:11 Marijn Suijten <[email protected]> napisał(a):
>
> Hi Marek,
>
> (Apologies for sending this to you twice; missed the reply-all button
> so this didn't end up on the mailing list)
>
> On Tue, 20 Oct 2020 at 09:31, Marek Czerski <[email protected]> wrote:
> >
> > Hi Marijn,
> >
> > pon., 19 paź 2020 o 21:53 Marijn Suijten <[email protected]> napisał(a):
> > >
> > > Hi Luiz, Marek,
> > >
> > > On Mon, 19 Oct 2020 at 18:47, Luiz Augusto von Dentz
> > > <[email protected]> wrote:
> > > >
> > > > Hi Marek, Marijn,
> > > >
> > > > On Mon, Oct 19, 2020 at 9:06 AM Marek Czerski <[email protected]> wrote:
> > > > >
> > > > > Hi Marijn,
> > > > >
> > > > > pon., 19 paź 2020 o 16:16 Marijn Suijten <[email protected]> napisał(a):
> > > > > >
> > > > > > 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
> > > > > > [email protected] [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.
> > > > > >
> > > > >
> > > > > I didn't tested Your patch but after looking at the code it seems that
> > > > > just applying Your patch would solve my problem.
> > >
> > > It should, even though I don't immediately see how swapping
> > > target/controller_init as per your initial email solves the problem.
> > >
> >
> > The sequence of events is important here. The
> > session->supported_events must be initialised (which is done in
> > target_init) before first AVRCP_REGISTER_NOTIFICATION command from the
> > remote side controller. If supported_events is not initialised and
> > AVRCP_REGISTER_NOTIFICATION arrives the
> > avrcp_handle_register_notification function rejects the event
> > registration. And this is what I see in wireshark.
>
> That's interesting and means we are dealing with two distinct problems
> here. No matter what order I flip controller/target_init in
> AVRCP_REGISTER_NOTIFICATION is always received after both functions
> complete. handle_vendordep_pdu is registered before these
> initialization functions making this possible though. I wonder if PDUs
> are dropped when the callback is registered later, after init. Perhaps
> controller/target_init need to be split in two, a static
> initialization part based on the remote profile version followed by
> registering this callback and finally functions that send commands to
> the remote like avrcp_get_capabilities.
>
> > And in my case, calling target_init earlier (the is before
> > controller_init) solves the problem because supported_events is
> > already initialised at the time the AVRCP_REGISTER_NOTIFICATION
> > arrives.
>
> That means your phone reports an AVRCP remote version of at least
> 0x104, otherwise supported_events would not contain
> AVRCP_EVENT_VOLUME_CHANGED. Can you validate this in wireshark or
> DBG("%p version 0x%04x", ...) in the logs? I'm seeing:
>
> profiles/audio/avrcp.c:target_init() 0x5614a2cbab70 version 0x0103
>
Yes, I get this in my logs when connecting from samsung galaxy S7:
profiles/audio/avrcp.c:target_init() 0x1103758 version 0x0104
profiles/audio/avrcp.c:controller_init() 0x1112c18 version 0x0104
> Either way the split Luiz and I mentioned mean one variant of
> supported_events (the one relevant for registering
> AVRCP_EVENT_VOLUME_CHANGED) is initialized in controller_init.
>
> > But I think that the problem is more general. There is some gap
> > between AVCTP connection establishment and BlueZ readiness to handle
> > commands from the remote side. There could be devices that send
> > commands during this gap and those devices would not work correctly
> > with BlueZ despite the fact that they are consistent with the
> > specification (or not ?).
> >
> > > > > Regarding avrcp version, in android there is developer option to set
> > > > > avrcp version. For example my Xiaomi redmi 8 (android 10) reports
> > > > > version according to this setting, but samsung galaxy s7 (android 8)
> > > > > always report version 1.4 regardless of this setting.
> > >
> > > I think this is the version used for the AVRCP Remote Target [1]. The
> > > version linked above, the one that is causing problems here is that of
> > > the AVRCP controller.
> > >
> > > There have always been problems changing these settings in developer
> > > options - sometimes they work, sometimes they don't. The system has
> > > been overhauled for Android 11 and is now much more consistent though
> > > avrcp still runs through a property. If I remember correctly
> > > restarting bluetooth or re-adding a device (in case of SDP caching) is
> > > the only solution.
> > >
> > > > We need to rework a little bit how the controller/target works, this
> > > > roles are actually supposed to be interpreted as client/server and
> > > > much like GATT they can be used simultaneously, so we need a target
> > > > versions and a controller version and independent supported events.
> > >
> > > Yes, I think that's the change we proposed last time and are proposing
> > > now. Unfortunately relevant functions need to know if the local
> > > (BlueZ) end is operating as target or controller for that particular
> > > operation.
> > >
> > > > That said if the remote side controller version does not indicate 1.4
> > > > or later we obviously can't support absolute volume control as that is
> > > > reserved in earlier versions.
> > >
> > > That depends on the way audio is flowing. When the local (BlueZ) end
> > > is the sink (rendering the audio) meaning it takes the role of AVRCP
> > > controller (the situation in the mail from Marek), the remote must be
> > > the AVRCP target and hence its controller version is irrelevant.
> > > Likewise when BlueZ is the audio source (AVRCP target), it should only
> > > care about the remote controller profile and version.
> > > But that is exactly what you described above: BlueZ needs to attain
> > > separation between handling notification registrations (and perhaps
> > > other code) as a controller and target separately.
> > >
> >
> > I don't know if I'm interpreting your words correctly but I think that
> > You make an assumption that a2dp sink must be avrcp CT and a2dp source
> > is avrcp TG.
> > In case of the absolute volume control it is not true. For absolute
> > volume control to work it is the other way around which makes both
> > sides be CT and TG at the same time.
>
> Quite the contrary, in my previous mail I attempted to explain
> multiple times that the CT/TG designation in the spec is used for
> initiator and target respectively, not the AVRCP controller and target
> profile. Indeed, depending on the transaction either side (controller
> or target) can be the initiator (CT). In other words CT and TG are
> _not_ abbreviations for the controller and target profile.
>
> > a2dp sink is avrcp CT for
> > controlling playback and a2dp source is avrcp CT for setting the
> > volume. It is the a2dp source that is sending
> > AVRCP_SET_ABSOLUTE_VOLUME commands.
>
> Correct, yet the A2DP sink will always take on the AVRCP controller
> profile where the phone being the A2DP source takes the AVRCP target
> profile.
>
> > > > > > 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.
> > > > > >
> > > > >
> > > > > One thing is not clear for me, what is the purpose of the
> > > > > supported_events ? It is used in two places:
> > > > > First is the avrcp_handle_register_notification function. If the
> > > > > remote side want to register itself for specific event notification it
> > > > > does not matter what version of avrcp that remote side supports. If it
> > > > > ask for specific event it clearly support that event.
> > >
> > > Looking at commit 4ae6135 that introduced this check it was apparently
> > > causing crashes without. I can only guess that devices were sending
> > > vendor-specific notification requests, assuming implementations are
> > > supposed to filter out anything beyond what their reported version
> > > supports? Either way input validation is still a sane thing to do.
> > >
> > > > > Second is in avrcp_handle_get_capabilities in CAP_EVENTS_SUPPORTED
> > > > > case. Does it matter if local side reply with events that are not
> > > > > supported in the version of avrcp supported by the remote side ?
> > >
> > > I guess it is sane here to not report capabilities the CT should -
> > > given it's version - not know about. I could not find anything in the
> > > 1.6.2 spec mandating this though, perhaps Luiz knows.
> > >
> > > > As I said we need to split the supported events to
> > > > ct(client)/tg(server) to avoid interpreting them as the same, it is
> > > > very odd that the remote would have different versions for each role
> > >
> > > Yeah, Android exposes different versions for its AVRCP Remote and
> > > AVRCP Remote Target.
> > > This is slightly confusing though: we discussed earlier that CT and TG
> > > is about the initiator and receiver respectively, with no direct
> > > mapping to AVRCP remote and target as either side can be the initiator
> > > (sender) of a command. Correct?
> > > In this specific case a notification registration will always be
> > > initiated by a CT and fulfilled by the TG, no matter whether it is an
> > > AVRCP remote or target.
> > >
> > > > but it looks like this is happening in this case although it is work
> > > > confirming if the CT version if in fact 1.3 as well we cannot enable
> > > > absolute volume control as that is not supported by that version, what
> > > > we can perhaps is to detect if SetAbsoluteVolume control is used then
> > > > update the events for the session.
> > >
> > > Again, in the specific case of this issue it is fine if the CT
> > > (initiator, the Android phone, supposed to behave like the target as
> > > it is the audio source) reports a controller version of 1.3 as we are
> > > only concerned about the target version, which will be the one
> > > registering for EVENT_VOLUME_CHANGED notifications from the
> > > sink/controller (BlueZ). Android always reports the AVRCP Remote
> > > Target as 1.4 or higher based on developer settings [1].
> > >
> > > > > > 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
> > > > >
> > > > >
> > > > > Best regards,
> > > > > Marek Czerski
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> > >
> > > P.S.: I hope it is fine to respond to two emails in one, seems like
> > > that will get confusing real quick if depth increases.
> > >
> > > - Marijn
> > >
> > > [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#612
> >
> > Best regards,
> > Marek
>
> - Marijn
--
mgr inż. Marek Czerski
+48 696 842 686
Hi Marijn,
Regarding the AVRCP version reported by the phone. There is something
weird going on on android regarding avrcp version and mac addresses
and absolute volume. I discovered couple of whings:
1. galaxy S7 will blacklist some mac addresses to not use absolute
volume for that device (checked it with "adb shell dumpsys
bluetooth_manager"). Changing mac addres of the device to something
other allows for absolute volume to work. For both cases it reports
avrcp version 1.4.
2. xiaomi redmi note 4 will report different avrcp version depending
on mac address. version 1.3 for mac address that galaxy s7 blacklists
and 1.6 for mac address that galaxy s7 does not blacklist. But in both
cases the absolute volume does not work ...
3. xiaomi redmi 8 will not even report that it has "A/V_RemoteControl"
(UUID 0x110E), it just reports that it has only
"A/V_RemoteControlTarget" (UUID 0x110C) but still it can control
volume on my yamaha av receiver and sony headphones and receive volume
updates from them. Very, very wierd.
For the record, to check the avrcp version of the phone I use sdptool
browse command (and to be 100% sure I check also the raw communication
on wireshark). For example this is one of the records reported for
xiaomi redmi note 4 and avrcp version 1.6:
Service RecHandle: 0x10007
Service Class ID List:
"AV Remote" (0x110e)
"AV Remote Controller" (0x110f)
Protocol Descriptor List:
"L2CAP" (0x0100)
PSM: 23
"AVCTP" (0x0017)
uint16: 0x0104
Profile Descriptor List:
"AV Remote" (0x110e)
Version: 0x0106
If there is something more I can do to help please let me know. For
example I can send hci snoop logs for specific cases etc.
wt., 20 paź 2020 o 15:12 Marek Czerski <[email protected]> napisał(a):
>
> Hi Marijn,
>
> wt., 20 paź 2020 o 11:11 Marijn Suijten <[email protected]> napisał(a):
> >
> > Hi Marek,
> >
> > (Apologies for sending this to you twice; missed the reply-all button
> > so this didn't end up on the mailing list)
> >
> > On Tue, 20 Oct 2020 at 09:31, Marek Czerski <[email protected]> wrote:
> > >
> > > Hi Marijn,
> > >
> > > pon., 19 paź 2020 o 21:53 Marijn Suijten <[email protected]> napisał(a):
> > > >
> > > > Hi Luiz, Marek,
> > > >
> > > > On Mon, 19 Oct 2020 at 18:47, Luiz Augusto von Dentz
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hi Marek, Marijn,
> > > > >
> > > > > On Mon, Oct 19, 2020 at 9:06 AM Marek Czerski <[email protected]> wrote:
> > > > > >
> > > > > > Hi Marijn,
> > > > > >
> > > > > > pon., 19 paź 2020 o 16:16 Marijn Suijten <[email protected]> napisał(a):
> > > > > > >
> > > > > > > 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
> > > > > > > [email protected] [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.
> > > > > > >
> > > > > >
> > > > > > I didn't tested Your patch but after looking at the code it seems that
> > > > > > just applying Your patch would solve my problem.
> > > >
> > > > It should, even though I don't immediately see how swapping
> > > > target/controller_init as per your initial email solves the problem.
> > > >
> > >
> > > The sequence of events is important here. The
> > > session->supported_events must be initialised (which is done in
> > > target_init) before first AVRCP_REGISTER_NOTIFICATION command from the
> > > remote side controller. If supported_events is not initialised and
> > > AVRCP_REGISTER_NOTIFICATION arrives the
> > > avrcp_handle_register_notification function rejects the event
> > > registration. And this is what I see in wireshark.
> >
> > That's interesting and means we are dealing with two distinct problems
> > here. No matter what order I flip controller/target_init in
> > AVRCP_REGISTER_NOTIFICATION is always received after both functions
> > complete. handle_vendordep_pdu is registered before these
> > initialization functions making this possible though. I wonder if PDUs
> > are dropped when the callback is registered later, after init. Perhaps
> > controller/target_init need to be split in two, a static
> > initialization part based on the remote profile version followed by
> > registering this callback and finally functions that send commands to
> > the remote like avrcp_get_capabilities.
> >
> > > And in my case, calling target_init earlier (the is before
> > > controller_init) solves the problem because supported_events is
> > > already initialised at the time the AVRCP_REGISTER_NOTIFICATION
> > > arrives.
> >
> > That means your phone reports an AVRCP remote version of at least
> > 0x104, otherwise supported_events would not contain
> > AVRCP_EVENT_VOLUME_CHANGED. Can you validate this in wireshark or
> > DBG("%p version 0x%04x", ...) in the logs? I'm seeing:
> >
> > profiles/audio/avrcp.c:target_init() 0x5614a2cbab70 version 0x0103
> >
>
> Yes, I get this in my logs when connecting from samsung galaxy S7:
> profiles/audio/avrcp.c:target_init() 0x1103758 version 0x0104
> profiles/audio/avrcp.c:controller_init() 0x1112c18 version 0x0104
>
> > Either way the split Luiz and I mentioned mean one variant of
> > supported_events (the one relevant for registering
> > AVRCP_EVENT_VOLUME_CHANGED) is initialized in controller_init.
> >
> > > But I think that the problem is more general. There is some gap
> > > between AVCTP connection establishment and BlueZ readiness to handle
> > > commands from the remote side. There could be devices that send
> > > commands during this gap and those devices would not work correctly
> > > with BlueZ despite the fact that they are consistent with the
> > > specification (or not ?).
> > >
> > > > > > Regarding avrcp version, in android there is developer option to set
> > > > > > avrcp version. For example my Xiaomi redmi 8 (android 10) reports
> > > > > > version according to this setting, but samsung galaxy s7 (android 8)
> > > > > > always report version 1.4 regardless of this setting.
> > > >
> > > > I think this is the version used for the AVRCP Remote Target [1]. The
> > > > version linked above, the one that is causing problems here is that of
> > > > the AVRCP controller.
> > > >
> > > > There have always been problems changing these settings in developer
> > > > options - sometimes they work, sometimes they don't. The system has
> > > > been overhauled for Android 11 and is now much more consistent though
> > > > avrcp still runs through a property. If I remember correctly
> > > > restarting bluetooth or re-adding a device (in case of SDP caching) is
> > > > the only solution.
> > > >
> > > > > We need to rework a little bit how the controller/target works, this
> > > > > roles are actually supposed to be interpreted as client/server and
> > > > > much like GATT they can be used simultaneously, so we need a target
> > > > > versions and a controller version and independent supported events.
> > > >
> > > > Yes, I think that's the change we proposed last time and are proposing
> > > > now. Unfortunately relevant functions need to know if the local
> > > > (BlueZ) end is operating as target or controller for that particular
> > > > operation.
> > > >
> > > > > That said if the remote side controller version does not indicate 1.4
> > > > > or later we obviously can't support absolute volume control as that is
> > > > > reserved in earlier versions.
> > > >
> > > > That depends on the way audio is flowing. When the local (BlueZ) end
> > > > is the sink (rendering the audio) meaning it takes the role of AVRCP
> > > > controller (the situation in the mail from Marek), the remote must be
> > > > the AVRCP target and hence its controller version is irrelevant.
> > > > Likewise when BlueZ is the audio source (AVRCP target), it should only
> > > > care about the remote controller profile and version.
> > > > But that is exactly what you described above: BlueZ needs to attain
> > > > separation between handling notification registrations (and perhaps
> > > > other code) as a controller and target separately.
> > > >
> > >
> > > I don't know if I'm interpreting your words correctly but I think that
> > > You make an assumption that a2dp sink must be avrcp CT and a2dp source
> > > is avrcp TG.
> > > In case of the absolute volume control it is not true. For absolute
> > > volume control to work it is the other way around which makes both
> > > sides be CT and TG at the same time.
> >
> > Quite the contrary, in my previous mail I attempted to explain
> > multiple times that the CT/TG designation in the spec is used for
> > initiator and target respectively, not the AVRCP controller and target
> > profile. Indeed, depending on the transaction either side (controller
> > or target) can be the initiator (CT). In other words CT and TG are
> > _not_ abbreviations for the controller and target profile.
> >
> > > a2dp sink is avrcp CT for
> > > controlling playback and a2dp source is avrcp CT for setting the
> > > volume. It is the a2dp source that is sending
> > > AVRCP_SET_ABSOLUTE_VOLUME commands.
> >
> > Correct, yet the A2DP sink will always take on the AVRCP controller
> > profile where the phone being the A2DP source takes the AVRCP target
> > profile.
> >
> > > > > > > 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.
> > > > > > >
> > > > > >
> > > > > > One thing is not clear for me, what is the purpose of the
> > > > > > supported_events ? It is used in two places:
> > > > > > First is the avrcp_handle_register_notification function. If the
> > > > > > remote side want to register itself for specific event notification it
> > > > > > does not matter what version of avrcp that remote side supports. If it
> > > > > > ask for specific event it clearly support that event.
> > > >
> > > > Looking at commit 4ae6135 that introduced this check it was apparently
> > > > causing crashes without. I can only guess that devices were sending
> > > > vendor-specific notification requests, assuming implementations are
> > > > supposed to filter out anything beyond what their reported version
> > > > supports? Either way input validation is still a sane thing to do.
> > > >
> > > > > > Second is in avrcp_handle_get_capabilities in CAP_EVENTS_SUPPORTED
> > > > > > case. Does it matter if local side reply with events that are not
> > > > > > supported in the version of avrcp supported by the remote side ?
> > > >
> > > > I guess it is sane here to not report capabilities the CT should -
> > > > given it's version - not know about. I could not find anything in the
> > > > 1.6.2 spec mandating this though, perhaps Luiz knows.
> > > >
> > > > > As I said we need to split the supported events to
> > > > > ct(client)/tg(server) to avoid interpreting them as the same, it is
> > > > > very odd that the remote would have different versions for each role
> > > >
> > > > Yeah, Android exposes different versions for its AVRCP Remote and
> > > > AVRCP Remote Target.
> > > > This is slightly confusing though: we discussed earlier that CT and TG
> > > > is about the initiator and receiver respectively, with no direct
> > > > mapping to AVRCP remote and target as either side can be the initiator
> > > > (sender) of a command. Correct?
> > > > In this specific case a notification registration will always be
> > > > initiated by a CT and fulfilled by the TG, no matter whether it is an
> > > > AVRCP remote or target.
> > > >
> > > > > but it looks like this is happening in this case although it is work
> > > > > confirming if the CT version if in fact 1.3 as well we cannot enable
> > > > > absolute volume control as that is not supported by that version, what
> > > > > we can perhaps is to detect if SetAbsoluteVolume control is used then
> > > > > update the events for the session.
> > > >
> > > > Again, in the specific case of this issue it is fine if the CT
> > > > (initiator, the Android phone, supposed to behave like the target as
> > > > it is the audio source) reports a controller version of 1.3 as we are
> > > > only concerned about the target version, which will be the one
> > > > registering for EVENT_VOLUME_CHANGED notifications from the
> > > > sink/controller (BlueZ). Android always reports the AVRCP Remote
> > > > Target as 1.4 or higher based on developer settings [1].
> > > >
> > > > > > > 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
> > > > > >
> > > > > >
> > > > > > Best regards,
> > > > > > Marek Czerski
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > > >
> > > > P.S.: I hope it is fine to respond to two emails in one, seems like
> > > > that will get confusing real quick if depth increases.
> > > >
> > > > - Marijn
> > > >
> > > > [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#612
> > >
> > > Best regards,
> > > Marek
> >
> > - Marijn
>
>
>
> --
> mgr inż. Marek Czerski
> +48 696 842 686
--
mgr inż. Marek Czerski
+48 696 842 686
Hi Marek,
On Wed, 21 Oct 2020 at 16:02, Marek Czerski <[email protected]> wrote:
>
> Hi Marijn,
>
> Regarding the AVRCP version reported by the phone. There is something
> weird going on on android regarding avrcp version and mac addresses
> and absolute volume. I discovered couple of whings:
> 1. galaxy S7 will blacklist some mac addresses to not use absolute
> volume for that device (checked it with "adb shell dumpsys
> bluetooth_manager"). Changing mac addres of the device to something
> other allows for absolute volume to work. For both cases it reports
> avrcp version 1.4.
There is indeed a database with traits based on mac-addresses [1].
> 2. xiaomi redmi note 4 will report different avrcp version depending
> on mac address. version 1.3 for mac address that galaxy s7 blacklists
> and 1.6 for mac address that galaxy s7 does not blacklist. But in both
> cases the absolute volume does not work ...
You confirmed earlier that besides this versioning issue there's also
an initialization issue with supported_events. I assume this test was
without flipping the _init calls around?
> 3. xiaomi redmi 8 will not even report that it has "A/V_RemoteControl"
> (UUID 0x110E), it just reports that it has only
> "A/V_RemoteControlTarget" (UUID 0x110C) but still it can control
> volume on my yamaha av receiver and sony headphones and receive volume
> updates from them. Very, very wierd.
That's not really surprising to me, in fact it is slightly weird that
other phones report the A/V_RemoteControl profile. A phone will always
be the audio source (thus the RemoteControlTarget), never the sink.
(Unless applying some hacks to libfluoride: it is possible to use an
older phone as A2DP sink - for example to connect to a receiver at the
other side of the room. I would have enabled it on all my phones if it
didn't break A2DP source capabilities at the same time... Maybe
Gabeldorsche addresses that)
> For the record, to check the avrcp version of the phone I use sdptool
> browse command (and to be 100% sure I check also the raw communication
> on wireshark). For example this is one of the records reported for
> xiaomi redmi note 4 and avrcp version 1.6:
> Service RecHandle: 0x10007
> Service Class ID List:
> "AV Remote" (0x110e)
> "AV Remote Controller" (0x110f)
> Protocol Descriptor List:
> "L2CAP" (0x0100)
> PSM: 23
> "AVCTP" (0x0017)
> uint16: 0x0104
> Profile Descriptor List:
> "AV Remote" (0x110e)
> Version: 0x0106
> If there is something more I can do to help please let me know. For
> example I can send hci snoop logs for specific cases etc.
I will send a _preliminary_ patch to this thread that should resolve
both the versioning and ordering issue as I've finally figured a way
to check whether the session associated with an AVRCP command is
running as source or sink. It is not finalized yet so please don't be
too hard on it :)
> wt., 20 paź 2020 o 15:12 Marek Czerski <[email protected]> napisał(a):
> >
> > Hi Marijn,
> >
> > wt., 20 paź 2020 o 11:11 Marijn Suijten <[email protected]> napisał(a):
> > >
> > > Hi Marek,
> > >
> > > (Apologies for sending this to you twice; missed the reply-all button
> > > so this didn't end up on the mailing list)
> > >
> > > On Tue, 20 Oct 2020 at 09:31, Marek Czerski <[email protected]> wrote:
> > > >
> > > > Hi Marijn,
> > > >
> > > > pon., 19 paź 2020 o 21:53 Marijn Suijten <[email protected]> napisał(a):
> > > > >
> > > > > Hi Luiz, Marek,
> > > > >
> > > > > On Mon, 19 Oct 2020 at 18:47, Luiz Augusto von Dentz
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > Hi Marek, Marijn,
> > > > > >
> > > > > > On Mon, Oct 19, 2020 at 9:06 AM Marek Czerski <[email protected]> wrote:
> > > > > > >
> > > > > > > Hi Marijn,
> > > > > > >
> > > > > > > pon., 19 paź 2020 o 16:16 Marijn Suijten <[email protected]> napisał(a):
> > > > > > > >
> > > > > > > > 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
> > > > > > > > [email protected] [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.
> > > > > > > >
> > > > > > >
> > > > > > > I didn't tested Your patch but after looking at the code it seems that
> > > > > > > just applying Your patch would solve my problem.
> > > > >
> > > > > It should, even though I don't immediately see how swapping
> > > > > target/controller_init as per your initial email solves the problem.
> > > > >
> > > >
> > > > The sequence of events is important here. The
> > > > session->supported_events must be initialised (which is done in
> > > > target_init) before first AVRCP_REGISTER_NOTIFICATION command from the
> > > > remote side controller. If supported_events is not initialised and
> > > > AVRCP_REGISTER_NOTIFICATION arrives the
> > > > avrcp_handle_register_notification function rejects the event
> > > > registration. And this is what I see in wireshark.
> > >
> > > That's interesting and means we are dealing with two distinct problems
> > > here. No matter what order I flip controller/target_init in
> > > AVRCP_REGISTER_NOTIFICATION is always received after both functions
> > > complete. handle_vendordep_pdu is registered before these
> > > initialization functions making this possible though. I wonder if PDUs
> > > are dropped when the callback is registered later, after init. Perhaps
> > > controller/target_init need to be split in two, a static
> > > initialization part based on the remote profile version followed by
> > > registering this callback and finally functions that send commands to
> > > the remote like avrcp_get_capabilities.
> > >
> > > > And in my case, calling target_init earlier (the is before
> > > > controller_init) solves the problem because supported_events is
> > > > already initialised at the time the AVRCP_REGISTER_NOTIFICATION
> > > > arrives.
> > >
> > > That means your phone reports an AVRCP remote version of at least
> > > 0x104, otherwise supported_events would not contain
> > > AVRCP_EVENT_VOLUME_CHANGED. Can you validate this in wireshark or
> > > DBG("%p version 0x%04x", ...) in the logs? I'm seeing:
> > >
> > > profiles/audio/avrcp.c:target_init() 0x5614a2cbab70 version 0x0103
> > >
> >
> > Yes, I get this in my logs when connecting from samsung galaxy S7:
> > profiles/audio/avrcp.c:target_init() 0x1103758 version 0x0104
> > profiles/audio/avrcp.c:controller_init() 0x1112c18 version 0x0104
> >
> > > Either way the split Luiz and I mentioned mean one variant of
> > > supported_events (the one relevant for registering
> > > AVRCP_EVENT_VOLUME_CHANGED) is initialized in controller_init.
> > >
> > > > But I think that the problem is more general. There is some gap
> > > > between AVCTP connection establishment and BlueZ readiness to handle
> > > > commands from the remote side. There could be devices that send
> > > > commands during this gap and those devices would not work correctly
> > > > with BlueZ despite the fact that they are consistent with the
> > > > specification (or not ?).
> > > >
> > > > > > > Regarding avrcp version, in android there is developer option to set
> > > > > > > avrcp version. For example my Xiaomi redmi 8 (android 10) reports
> > > > > > > version according to this setting, but samsung galaxy s7 (android 8)
> > > > > > > always report version 1.4 regardless of this setting.
> > > > >
> > > > > I think this is the version used for the AVRCP Remote Target [1]. The
> > > > > version linked above, the one that is causing problems here is that of
> > > > > the AVRCP controller.
> > > > >
> > > > > There have always been problems changing these settings in developer
> > > > > options - sometimes they work, sometimes they don't. The system has
> > > > > been overhauled for Android 11 and is now much more consistent though
> > > > > avrcp still runs through a property. If I remember correctly
> > > > > restarting bluetooth or re-adding a device (in case of SDP caching) is
> > > > > the only solution.
> > > > >
> > > > > > We need to rework a little bit how the controller/target works, this
> > > > > > roles are actually supposed to be interpreted as client/server and
> > > > > > much like GATT they can be used simultaneously, so we need a target
> > > > > > versions and a controller version and independent supported events.
> > > > >
> > > > > Yes, I think that's the change we proposed last time and are proposing
> > > > > now. Unfortunately relevant functions need to know if the local
> > > > > (BlueZ) end is operating as target or controller for that particular
> > > > > operation.
> > > > >
> > > > > > That said if the remote side controller version does not indicate 1.4
> > > > > > or later we obviously can't support absolute volume control as that is
> > > > > > reserved in earlier versions.
> > > > >
> > > > > That depends on the way audio is flowing. When the local (BlueZ) end
> > > > > is the sink (rendering the audio) meaning it takes the role of AVRCP
> > > > > controller (the situation in the mail from Marek), the remote must be
> > > > > the AVRCP target and hence its controller version is irrelevant.
> > > > > Likewise when BlueZ is the audio source (AVRCP target), it should only
> > > > > care about the remote controller profile and version.
> > > > > But that is exactly what you described above: BlueZ needs to attain
> > > > > separation between handling notification registrations (and perhaps
> > > > > other code) as a controller and target separately.
> > > > >
> > > >
> > > > I don't know if I'm interpreting your words correctly but I think that
> > > > You make an assumption that a2dp sink must be avrcp CT and a2dp source
> > > > is avrcp TG.
> > > > In case of the absolute volume control it is not true. For absolute
> > > > volume control to work it is the other way around which makes both
> > > > sides be CT and TG at the same time.
> > >
> > > Quite the contrary, in my previous mail I attempted to explain
> > > multiple times that the CT/TG designation in the spec is used for
> > > initiator and target respectively, not the AVRCP controller and target
> > > profile. Indeed, depending on the transaction either side (controller
> > > or target) can be the initiator (CT). In other words CT and TG are
> > > _not_ abbreviations for the controller and target profile.
> > >
> > > > a2dp sink is avrcp CT for
> > > > controlling playback and a2dp source is avrcp CT for setting the
> > > > volume. It is the a2dp source that is sending
> > > > AVRCP_SET_ABSOLUTE_VOLUME commands.
> > >
> > > Correct, yet the A2DP sink will always take on the AVRCP controller
> > > profile where the phone being the A2DP source takes the AVRCP target
> > > profile.
> > >
> > > > > > > > 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.
> > > > > > > >
> > > > > > >
> > > > > > > One thing is not clear for me, what is the purpose of the
> > > > > > > supported_events ? It is used in two places:
> > > > > > > First is the avrcp_handle_register_notification function. If the
> > > > > > > remote side want to register itself for specific event notification it
> > > > > > > does not matter what version of avrcp that remote side supports. If it
> > > > > > > ask for specific event it clearly support that event.
> > > > >
> > > > > Looking at commit 4ae6135 that introduced this check it was apparently
> > > > > causing crashes without. I can only guess that devices were sending
> > > > > vendor-specific notification requests, assuming implementations are
> > > > > supposed to filter out anything beyond what their reported version
> > > > > supports? Either way input validation is still a sane thing to do.
> > > > >
> > > > > > > Second is in avrcp_handle_get_capabilities in CAP_EVENTS_SUPPORTED
> > > > > > > case. Does it matter if local side reply with events that are not
> > > > > > > supported in the version of avrcp supported by the remote side ?
> > > > >
> > > > > I guess it is sane here to not report capabilities the CT should -
> > > > > given it's version - not know about. I could not find anything in the
> > > > > 1.6.2 spec mandating this though, perhaps Luiz knows.
> > > > >
> > > > > > As I said we need to split the supported events to
> > > > > > ct(client)/tg(server) to avoid interpreting them as the same, it is
> > > > > > very odd that the remote would have different versions for each role
> > > > >
> > > > > Yeah, Android exposes different versions for its AVRCP Remote and
> > > > > AVRCP Remote Target.
> > > > > This is slightly confusing though: we discussed earlier that CT and TG
> > > > > is about the initiator and receiver respectively, with no direct
> > > > > mapping to AVRCP remote and target as either side can be the initiator
> > > > > (sender) of a command. Correct?
> > > > > In this specific case a notification registration will always be
> > > > > initiated by a CT and fulfilled by the TG, no matter whether it is an
> > > > > AVRCP remote or target.
> > > > >
> > > > > > but it looks like this is happening in this case although it is work
> > > > > > confirming if the CT version if in fact 1.3 as well we cannot enable
> > > > > > absolute volume control as that is not supported by that version, what
> > > > > > we can perhaps is to detect if SetAbsoluteVolume control is used then
> > > > > > update the events for the session.
> > > > >
> > > > > Again, in the specific case of this issue it is fine if the CT
> > > > > (initiator, the Android phone, supposed to behave like the target as
> > > > > it is the audio source) reports a controller version of 1.3 as we are
> > > > > only concerned about the target version, which will be the one
> > > > > registering for EVENT_VOLUME_CHANGED notifications from the
> > > > > sink/controller (BlueZ). Android always reports the AVRCP Remote
> > > > > Target as 1.4 or higher based on developer settings [1].
> > > > >
> > > > > > > > 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
> > > > > > >
> > > > > > >
> > > > > > > Best regards,
> > > > > > > Marek Czerski
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Luiz Augusto von Dentz
> > > > >
> > > > > P.S.: I hope it is fine to respond to two emails in one, seems like
> > > > > that will get confusing real quick if depth increases.
> > > > >
> > > > > - Marijn
> > > > >
> > > > > [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#612
> > > >
> > > > Best regards,
> > > > Marek
> > >
> > > - Marijn
> >
> >
> >
> > --
> > mgr inż. Marek Czerski
> > +48 696 842 686
>
>
>
> --
> mgr inż. Marek Czerski
> +48 696 842 686
- Marijn
[1]: https://android.googlesource.com/platform/system/bt/+/master/device/include/interop_database.h
supported_events was previously initialized based on whatever the AV
Remote Controller profile running on the peer device could request based
on its version, and assumes BlueZ is running in the AV Remote Target
profile.
If however BlueZ runs the Remote Controller profile (is an audio sink)
against a Remote Target profile on the peer (the audio source) this
version is incorrectly taken into account: a Remote Controller profile
on the peer is not involved in this connection. If its version is too
low supported_events will not contain all events the peer might
rightfully attempt to register.
This is particularly problematic with Android phones as an A2DP audio
source playing back to BlueZ which is the sink. Most Android phones
report their Remote Controller profile version as 1.3 when initializing
as audio source [1] meaning that AVRCP_EVENT_VOLUME_CHANGED is
inadvertently rejected in avrcp_handle_register_notification. As
mentioned above this profile is of no relevance to the connection, only
the Remote Target on the phone (source) and Remote Controller on BlueZ
(sink) take part.
The problem is addressed by splitting supported_events in two variables:
target_supported_events containing all events the peer Remote Controller
might attempt to register with the local Remote Target profile, and
controller_supported_events containing all events the Remote Target
might attempt to register with the local Remote Controller profile.
In addition the possible notifications going either way have been
limited. An audio source is in control over media playback and will
never request playback state changes from the Remote Controller.
Likewise the audio sink is in control over its rendering volume and will
never have to request volume notifications from the Remote Target.
This does however still allow the Remote Controller to send playback
control messages to the source, and the Remote Target to send
SetAbsoluteVolume to the sink; both of which are not notifications.
[1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#761
---
Hi Luiz, Marek,
This is a preliminary version of the patch that aims to address the
issues covered in our mail thread. Keep in mind it is "intentionally"
messy; I commented out unexpected events based on logically deriving the
possibilities (as described in the message above) without checking if
this is in accordance with the documentation.
I still have to test this between two devices that can both run an audio
sink and source, such as two machines running BlueZ. Playing back audio
both ways should never make this collapse on its own, though I think at
that point multiple transports are associated with a device and
media_transport_is_source would be lying, based on whichever transport
comes first in the list. Likewise registered_events needs to be split
in two variables as well.
I'm not sure what's causing the race condition Marek was observing. I
assume the call to avrcp_get_capabilities or avrcp_connect_browsing in
controller_init triggers the peer to start requesting capabilities and
registering for notifications, before target_init is called (which would
previously be too late to initialize supported_events). That case will
also be addressed in this patch, but if it happens "at random" because
the pdu handler is registered before supported_events is assigned I
propose to split session_init_control in 3 steps instead:
1. Retrieve remote profile versions and set up *_supported_events;
2. Register pdu and passthrough handler;
3. The rest from {controller,target}_init.
Looking forward to hearing your suggestions.
Best regards,
Marijn Suijten
profiles/audio/avrcp.c | 58 +++++++++++++++++++++++++++++++-------
profiles/audio/transport.c | 20 +++++++++++++
profiles/audio/transport.h | 1 +
3 files changed, 69 insertions(+), 10 deletions(-)
diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
index c093deac8..af5dc4212 100644
--- a/profiles/audio/avrcp.c
+++ b/profiles/audio/avrcp.c
@@ -269,7 +269,13 @@ struct avrcp {
unsigned int control_id;
unsigned int browsing_id;
unsigned int browsing_timer;
- uint16_t supported_events;
+ // TODO: Swap names to make them represent the name of the peer profile,
+ // instead of the opposite local profile?
+ /* Events the Remote Target expects based on peer Remote Controller version */
+ uint16_t target_supported_events;
+ /* Events the Remote Controller expects based on peer Remote Target version */
+ uint16_t controller_supported_events;
+ // TODO: Registered_events should be split across controller/target too!
uint16_t registered_events;
uint8_t transaction;
uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
@@ -1060,7 +1066,7 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
struct avrcp_header *pdu,
uint8_t transaction)
{
- uint16_t len = ntohs(pdu->params_len);
+ uint16_t len = ntohs(pdu->params_len), supported_events;
unsigned int i;
if (len != 1)
@@ -1068,6 +1074,11 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
DBG("id=%u", pdu->params[0]);
+ if (media_transport_is_source(session->dev))
+ supported_events = session->target_supported_events;
+ else
+ supported_events = session->controller_supported_events;
+
switch (pdu->params[0]) {
case CAP_COMPANY_ID:
for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
@@ -1082,7 +1093,7 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
case CAP_EVENTS_SUPPORTED:
pdu->params[1] = 0;
for (i = 1; i <= AVRCP_EVENT_LAST; i++) {
- if (session->supported_events & (1 << i)) {
+ if (supported_events & (1 << i)) {
pdu->params[1]++;
pdu->params[pdu->params[1] + 1] = i;
}
@@ -1607,7 +1618,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
{
struct avrcp_player *player = target_get_player(session);
struct btd_device *dev = session->dev;
- uint16_t len = ntohs(pdu->params_len);
+ uint16_t len = ntohs(pdu->params_len), supported_events;
uint64_t uid;
int8_t volume;
@@ -1620,7 +1631,12 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
goto err;
/* Check if event is supported otherwise reject */
- if (!(session->supported_events & (1 << pdu->params[0])))
+ if (media_transport_is_source(session->dev))
+ supported_events = session->target_supported_events;
+ else
+ supported_events = session->controller_supported_events;
+
+ if (!(supported_events & (1 << pdu->params[0])))
goto err;
switch (pdu->params[0]) {
@@ -4129,7 +4145,11 @@ static void target_init(struct avrcp *session)
media_transport_update_device_volume(session->dev, init_volume);
}
- session->supported_events |= (1 << AVRCP_EVENT_STATUS_CHANGED) |
+ if (target->version < 0x0103)
+ return;
+
+ session->target_supported_events |=
+ (1 << AVRCP_EVENT_STATUS_CHANGED) |
(1 << AVRCP_EVENT_TRACK_CHANGED) |
(1 << AVRCP_EVENT_TRACK_REACHED_START) |
(1 << AVRCP_EVENT_TRACK_REACHED_END) |
@@ -4138,10 +4158,13 @@ static void target_init(struct avrcp *session)
if (target->version < 0x0104)
return;
- session->supported_events |=
+ session->target_supported_events |=
(1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
- (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
- (1 << AVRCP_EVENT_VOLUME_CHANGED);
+ (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED);
+ // Does not make sense here; the remote is the
+ // rendering device and in control, it'll never
+ // request this notification.
+ // (1 << AVRCP_EVENT_VOLUME_CHANGED);
/* Only check capabilities if controller is not supported */
if (session->controller == NULL)
@@ -4180,11 +4203,26 @@ static void controller_init(struct avrcp *session)
if (controller->version < 0x0103)
return;
- avrcp_get_capabilities(session);
+ // Players should only run on the remote target; they
+ // should never request notifications about their own
+ // playback status.
+ // session->controller_supported_events |=
+ // (1 << AVRCP_EVENT_STATUS_CHANGED) |
+ // (1 << AVRCP_EVENT_TRACK_CHANGED) |
+ // (1 << AVRCP_EVENT_TRACK_REACHED_START) |
+ // (1 << AVRCP_EVENT_TRACK_REACHED_END) |
+ // (1 << AVRCP_EVENT_SETTINGS_CHANGED);
if (controller->version < 0x0104)
return;
+ session->controller_supported_events |=
+ // (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
+ // (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
+ (1 << AVRCP_EVENT_VOLUME_CHANGED);
+
+ avrcp_get_capabilities(session);
+
if (!(controller->features & AVRCP_FEATURE_BROWSING))
return;
diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
index 8248014ae..f5226776f 100644
--- a/profiles/audio/transport.c
+++ b/profiles/audio/transport.c
@@ -980,3 +980,23 @@ void media_transport_update_device_volume(struct btd_device *dev,
media_transport_update_volume(transport, volume);
}
}
+
+gboolean media_transport_is_source(struct btd_device *dev)
+{
+ GSList *l;
+ const char *uuid;
+
+ if (dev == NULL)
+ return FALSE;
+
+ for (l = transports; l; l = l->next) {
+ struct media_transport *transport = l->data;
+ if (transport->device != dev)
+ continue;
+
+ uuid = media_endpoint_get_uuid(transport->endpoint);
+ return strcasecmp(uuid, A2DP_SOURCE_UUID) == 0;
+ }
+
+ return FALSE;
+}
diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
index 51a67ea74..eb1621813 100644
--- a/profiles/audio/transport.h
+++ b/profiles/audio/transport.h
@@ -30,3 +30,4 @@ void transport_get_properties(struct media_transport *transport,
int8_t media_transport_get_device_volume(struct btd_device *dev);
void media_transport_update_device_volume(struct btd_device *dev,
int8_t volume);
+gboolean media_transport_is_source(struct btd_device *dev);
--
2.29.0
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=368467
---Test result---
##############################
Test: CheckPatch - FAIL
Output:
audio: avrcp: Split supported events between target and controller
WARNING:COMMIT_LOG_LONG_LINE: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#42:
[1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#761
WARNING:LONG_LINE_COMMENT: line over 80 characters
#55: FILE: profiles/audio/avrcp.c:274:
+ /* Events the Remote Target expects based on peer Remote Controller version */
WARNING:LONG_LINE_COMMENT: line over 80 characters
#57: FILE: profiles/audio/avrcp.c:276:
+ /* Events the Remote Controller expects based on peer Remote Target version */
WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#155: FILE: profiles/audio/avrcp.c:4210:
+^I// ^I^I^I(1 << AVRCP_EVENT_STATUS_CHANGED) |$
WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#156: FILE: profiles/audio/avrcp.c:4211:
+^I// ^I^I^I(1 << AVRCP_EVENT_TRACK_CHANGED) |$
WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#157: FILE: profiles/audio/avrcp.c:4212:
+^I// ^I^I^I(1 << AVRCP_EVENT_TRACK_REACHED_START) |$
WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#158: FILE: profiles/audio/avrcp.c:4213:
+^I// ^I^I^I(1 << AVRCP_EVENT_TRACK_REACHED_END) |$
WARNING:SPACE_BEFORE_TAB: please, no space before tabs
#159: FILE: profiles/audio/avrcp.c:4214:
+^I// ^I^I^I(1 << AVRCP_EVENT_SETTINGS_CHANGED);$
WARNING:LONG_LINE_COMMENT: line over 80 characters
#166: FILE: profiles/audio/avrcp.c:4221:
+ // (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
WARNING:LINE_SPACING: Missing a blank line after declarations
#193: FILE: profiles/audio/transport.c:994:
+ struct media_transport *transport = l->data;
+ if (transport->device != dev)
- total: 0 errors, 10 warnings, 144 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
Your patch has style problems, please review.
NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPLIT_STRING SSCANF_TO_KSTRTO
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: CheckGitLint - FAIL
Output:
audio: avrcp: Split supported events between target and controller
38: B1 Line exceeds max length (102>80): "[1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#761"
##############################
Test: CheckBuild - PASS
##############################
Test: MakeCheck - PASS
---
Regards,
Linux Bluetooth
Hi Marijn,
czw., 22 paź 2020 o 00:10 Marijn Suijten <[email protected]> napisał(a):
>
> Hi Marek,
>
> On Wed, 21 Oct 2020 at 16:02, Marek Czerski <[email protected]> wrote:
> >
> > Hi Marijn,
> >
> > Regarding the AVRCP version reported by the phone. There is something
> > weird going on on android regarding avrcp version and mac addresses
> > and absolute volume. I discovered couple of whings:
> > 1. galaxy S7 will blacklist some mac addresses to not use absolute
> > volume for that device (checked it with "adb shell dumpsys
> > bluetooth_manager"). Changing mac addres of the device to something
> > other allows for absolute volume to work. For both cases it reports
> > avrcp version 1.4.
>
> There is indeed a database with traits based on mac-addresses [1].
>
> > 2. xiaomi redmi note 4 will report different avrcp version depending
> > on mac address. version 1.3 for mac address that galaxy s7 blacklists
> > and 1.6 for mac address that galaxy s7 does not blacklist. But in both
> > cases the absolute volume does not work ...
>
> You confirmed earlier that besides this versioning issue there's also
> an initialization issue with supported_events. I assume this test was
> without flipping the _init calls around?
>
To tests if absolute volume is working on redmi note 4 I used two
different a2dp sink devices, yamaha rx-v485 av receiver and sony
wh-ch510 headphones. The AVRCP version was read from "sdptool browse"
output running on my linux machine.
So in this case, the absolute volume was not working at all on this
phone despite the fact that it should (developer options were set to
enable absolute volume). Also the absolute volume on this phone was
not working with bluez no matter what the order of init calls.
> > 3. xiaomi redmi 8 will not even report that it has "A/V_RemoteControl"
> > (UUID 0x110E), it just reports that it has only
> > "A/V_RemoteControlTarget" (UUID 0x110C) but still it can control
> > volume on my yamaha av receiver and sony headphones and receive volume
> > updates from them. Very, very wierd.
>
> That's not really surprising to me, in fact it is slightly weird that
> other phones report the A/V_RemoteControl profile. A phone will always
> be the audio source (thus the RemoteControlTarget), never the sink.
>
I know You were explaining that already but for me it is still a mystery ...
In AVRCP specification v1.6.2 (this is the newest one on the bluetooth
sig site) in paragraph 2.2.1 (Roles) there is:
"The controller (CT) is a device that initiates a transaction by
sending a command frame to a target."
So if the device wants to send a command it must play the controller
role. And SetAbsoluteVolume is the command that is sent from the phone
when you press volume up/down on the phone. So To support absolute
volume the phone has to be a controller (and the target to handle
play/pause etc. commands).
Next in the same specification in chapter 8 (Service discovery
interoperability requirements) there is:
"This profile defines the following service records for the CT and the
TG, respectively." and below is definition of those records.
So I understand it that if phone wants to send SetAbsoluteVolume
command it must implement the controller role (CT) and define the
service record with services "A/V Remote ControlI" (0x110E) and "A/V
Remote Control Controller" (0x110F) in "Service Class ID List" field.
And in case of redmi 8, this phone does not define such service record
and still it sends SetAbsoluteVolume commands.
So either this phone is not respecting the specification or I totally
missed the point. (I guess the second ...)
> (Unless applying some hacks to libfluoride: it is possible to use an
> older phone as A2DP sink - for example to connect to a receiver at the
> other side of the room. I would have enabled it on all my phones if it
> didn't break A2DP source capabilities at the same time... Maybe
> Gabeldorsche addresses that)
>
> > For the record, to check the avrcp version of the phone I use sdptool
> > browse command (and to be 100% sure I check also the raw communication
> > on wireshark). For example this is one of the records reported for
> > xiaomi redmi note 4 and avrcp version 1.6:
> > Service RecHandle: 0x10007
> > Service Class ID List:
> > "AV Remote" (0x110e)
> > "AV Remote Controller" (0x110f)
> > Protocol Descriptor List:
> > "L2CAP" (0x0100)
> > PSM: 23
> > "AVCTP" (0x0017)
> > uint16: 0x0104
> > Profile Descriptor List:
> > "AV Remote" (0x110e)
> > Version: 0x0106
> > If there is something more I can do to help please let me know. For
> > example I can send hci snoop logs for specific cases etc.
>
> I will send a _preliminary_ patch to this thread that should resolve
> both the versioning and ordering issue as I've finally figured a way
> to check whether the session associated with an AVRCP command is
> running as source or sink. It is not finalized yet so please don't be
> too hard on it :)
>
I will be happy to test it :)
> > wt., 20 paź 2020 o 15:12 Marek Czerski <[email protected]> napisał(a):
> > >
> > > Hi Marijn,
> > >
> > > wt., 20 paź 2020 o 11:11 Marijn Suijten <[email protected]> napisał(a):
> > > >
> > > > Hi Marek,
> > > >
> > > > (Apologies for sending this to you twice; missed the reply-all button
> > > > so this didn't end up on the mailing list)
> > > >
> > > > On Tue, 20 Oct 2020 at 09:31, Marek Czerski <[email protected]> wrote:
> > > > >
> > > > > Hi Marijn,
> > > > >
> > > > > pon., 19 paź 2020 o 21:53 Marijn Suijten <[email protected]> napisał(a):
> > > > > >
> > > > > > Hi Luiz, Marek,
> > > > > >
> > > > > > On Mon, 19 Oct 2020 at 18:47, Luiz Augusto von Dentz
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > Hi Marek, Marijn,
> > > > > > >
> > > > > > > On Mon, Oct 19, 2020 at 9:06 AM Marek Czerski <[email protected]> wrote:
> > > > > > > >
> > > > > > > > Hi Marijn,
> > > > > > > >
> > > > > > > > pon., 19 paź 2020 o 16:16 Marijn Suijten <[email protected]> napisał(a):
> > > > > > > > >
> > > > > > > > > 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
> > > > > > > > > [email protected] [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.
> > > > > > > > >
> > > > > > > >
> > > > > > > > I didn't tested Your patch but after looking at the code it seems that
> > > > > > > > just applying Your patch would solve my problem.
> > > > > >
> > > > > > It should, even though I don't immediately see how swapping
> > > > > > target/controller_init as per your initial email solves the problem.
> > > > > >
> > > > >
> > > > > The sequence of events is important here. The
> > > > > session->supported_events must be initialised (which is done in
> > > > > target_init) before first AVRCP_REGISTER_NOTIFICATION command from the
> > > > > remote side controller. If supported_events is not initialised and
> > > > > AVRCP_REGISTER_NOTIFICATION arrives the
> > > > > avrcp_handle_register_notification function rejects the event
> > > > > registration. And this is what I see in wireshark.
> > > >
> > > > That's interesting and means we are dealing with two distinct problems
> > > > here. No matter what order I flip controller/target_init in
> > > > AVRCP_REGISTER_NOTIFICATION is always received after both functions
> > > > complete. handle_vendordep_pdu is registered before these
> > > > initialization functions making this possible though. I wonder if PDUs
> > > > are dropped when the callback is registered later, after init. Perhaps
> > > > controller/target_init need to be split in two, a static
> > > > initialization part based on the remote profile version followed by
> > > > registering this callback and finally functions that send commands to
> > > > the remote like avrcp_get_capabilities.
> > > >
> > > > > And in my case, calling target_init earlier (the is before
> > > > > controller_init) solves the problem because supported_events is
> > > > > already initialised at the time the AVRCP_REGISTER_NOTIFICATION
> > > > > arrives.
> > > >
> > > > That means your phone reports an AVRCP remote version of at least
> > > > 0x104, otherwise supported_events would not contain
> > > > AVRCP_EVENT_VOLUME_CHANGED. Can you validate this in wireshark or
> > > > DBG("%p version 0x%04x", ...) in the logs? I'm seeing:
> > > >
> > > > profiles/audio/avrcp.c:target_init() 0x5614a2cbab70 version 0x0103
> > > >
> > >
> > > Yes, I get this in my logs when connecting from samsung galaxy S7:
> > > profiles/audio/avrcp.c:target_init() 0x1103758 version 0x0104
> > > profiles/audio/avrcp.c:controller_init() 0x1112c18 version 0x0104
> > >
> > > > Either way the split Luiz and I mentioned mean one variant of
> > > > supported_events (the one relevant for registering
> > > > AVRCP_EVENT_VOLUME_CHANGED) is initialized in controller_init.
> > > >
> > > > > But I think that the problem is more general. There is some gap
> > > > > between AVCTP connection establishment and BlueZ readiness to handle
> > > > > commands from the remote side. There could be devices that send
> > > > > commands during this gap and those devices would not work correctly
> > > > > with BlueZ despite the fact that they are consistent with the
> > > > > specification (or not ?).
> > > > >
> > > > > > > > Regarding avrcp version, in android there is developer option to set
> > > > > > > > avrcp version. For example my Xiaomi redmi 8 (android 10) reports
> > > > > > > > version according to this setting, but samsung galaxy s7 (android 8)
> > > > > > > > always report version 1.4 regardless of this setting.
> > > > > >
> > > > > > I think this is the version used for the AVRCP Remote Target [1]. The
> > > > > > version linked above, the one that is causing problems here is that of
> > > > > > the AVRCP controller.
> > > > > >
> > > > > > There have always been problems changing these settings in developer
> > > > > > options - sometimes they work, sometimes they don't. The system has
> > > > > > been overhauled for Android 11 and is now much more consistent though
> > > > > > avrcp still runs through a property. If I remember correctly
> > > > > > restarting bluetooth or re-adding a device (in case of SDP caching) is
> > > > > > the only solution.
> > > > > >
> > > > > > > We need to rework a little bit how the controller/target works, this
> > > > > > > roles are actually supposed to be interpreted as client/server and
> > > > > > > much like GATT they can be used simultaneously, so we need a target
> > > > > > > versions and a controller version and independent supported events.
> > > > > >
> > > > > > Yes, I think that's the change we proposed last time and are proposing
> > > > > > now. Unfortunately relevant functions need to know if the local
> > > > > > (BlueZ) end is operating as target or controller for that particular
> > > > > > operation.
> > > > > >
> > > > > > > That said if the remote side controller version does not indicate 1.4
> > > > > > > or later we obviously can't support absolute volume control as that is
> > > > > > > reserved in earlier versions.
> > > > > >
> > > > > > That depends on the way audio is flowing. When the local (BlueZ) end
> > > > > > is the sink (rendering the audio) meaning it takes the role of AVRCP
> > > > > > controller (the situation in the mail from Marek), the remote must be
> > > > > > the AVRCP target and hence its controller version is irrelevant.
> > > > > > Likewise when BlueZ is the audio source (AVRCP target), it should only
> > > > > > care about the remote controller profile and version.
> > > > > > But that is exactly what you described above: BlueZ needs to attain
> > > > > > separation between handling notification registrations (and perhaps
> > > > > > other code) as a controller and target separately.
> > > > > >
> > > > >
> > > > > I don't know if I'm interpreting your words correctly but I think that
> > > > > You make an assumption that a2dp sink must be avrcp CT and a2dp source
> > > > > is avrcp TG.
> > > > > In case of the absolute volume control it is not true. For absolute
> > > > > volume control to work it is the other way around which makes both
> > > > > sides be CT and TG at the same time.
> > > >
> > > > Quite the contrary, in my previous mail I attempted to explain
> > > > multiple times that the CT/TG designation in the spec is used for
> > > > initiator and target respectively, not the AVRCP controller and target
> > > > profile. Indeed, depending on the transaction either side (controller
> > > > or target) can be the initiator (CT). In other words CT and TG are
> > > > _not_ abbreviations for the controller and target profile.
> > > >
> > > > > a2dp sink is avrcp CT for
> > > > > controlling playback and a2dp source is avrcp CT for setting the
> > > > > volume. It is the a2dp source that is sending
> > > > > AVRCP_SET_ABSOLUTE_VOLUME commands.
> > > >
> > > > Correct, yet the A2DP sink will always take on the AVRCP controller
> > > > profile where the phone being the A2DP source takes the AVRCP target
> > > > profile.
> > > >
> > > > > > > > > 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.
> > > > > > > > >
> > > > > > > >
> > > > > > > > One thing is not clear for me, what is the purpose of the
> > > > > > > > supported_events ? It is used in two places:
> > > > > > > > First is the avrcp_handle_register_notification function. If the
> > > > > > > > remote side want to register itself for specific event notification it
> > > > > > > > does not matter what version of avrcp that remote side supports. If it
> > > > > > > > ask for specific event it clearly support that event.
> > > > > >
> > > > > > Looking at commit 4ae6135 that introduced this check it was apparently
> > > > > > causing crashes without. I can only guess that devices were sending
> > > > > > vendor-specific notification requests, assuming implementations are
> > > > > > supposed to filter out anything beyond what their reported version
> > > > > > supports? Either way input validation is still a sane thing to do.
> > > > > >
> > > > > > > > Second is in avrcp_handle_get_capabilities in CAP_EVENTS_SUPPORTED
> > > > > > > > case. Does it matter if local side reply with events that are not
> > > > > > > > supported in the version of avrcp supported by the remote side ?
> > > > > >
> > > > > > I guess it is sane here to not report capabilities the CT should -
> > > > > > given it's version - not know about. I could not find anything in the
> > > > > > 1.6.2 spec mandating this though, perhaps Luiz knows.
> > > > > >
> > > > > > > As I said we need to split the supported events to
> > > > > > > ct(client)/tg(server) to avoid interpreting them as the same, it is
> > > > > > > very odd that the remote would have different versions for each role
> > > > > >
> > > > > > Yeah, Android exposes different versions for its AVRCP Remote and
> > > > > > AVRCP Remote Target.
> > > > > > This is slightly confusing though: we discussed earlier that CT and TG
> > > > > > is about the initiator and receiver respectively, with no direct
> > > > > > mapping to AVRCP remote and target as either side can be the initiator
> > > > > > (sender) of a command. Correct?
> > > > > > In this specific case a notification registration will always be
> > > > > > initiated by a CT and fulfilled by the TG, no matter whether it is an
> > > > > > AVRCP remote or target.
> > > > > >
> > > > > > > but it looks like this is happening in this case although it is work
> > > > > > > confirming if the CT version if in fact 1.3 as well we cannot enable
> > > > > > > absolute volume control as that is not supported by that version, what
> > > > > > > we can perhaps is to detect if SetAbsoluteVolume control is used then
> > > > > > > update the events for the session.
> > > > > >
> > > > > > Again, in the specific case of this issue it is fine if the CT
> > > > > > (initiator, the Android phone, supposed to behave like the target as
> > > > > > it is the audio source) reports a controller version of 1.3 as we are
> > > > > > only concerned about the target version, which will be the one
> > > > > > registering for EVENT_VOLUME_CHANGED notifications from the
> > > > > > sink/controller (BlueZ). Android always reports the AVRCP Remote
> > > > > > Target as 1.4 or higher based on developer settings [1].
> > > > > >
> > > > > > > > > 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
> > > > > > > >
> > > > > > > >
> > > > > > > > Best regards,
> > > > > > > > Marek Czerski
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > --
> > > > > > > Luiz Augusto von Dentz
> > > > > >
> > > > > > P.S.: I hope it is fine to respond to two emails in one, seems like
> > > > > > that will get confusing real quick if depth increases.
> > > > > >
> > > > > > - Marijn
> > > > > >
> > > > > > [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#612
> > > > >
> > > > > Best regards,
> > > > > Marek
> > > >
> > > > - Marijn
> > >
> > >
> > >
> > > --
> > > mgr inż. Marek Czerski
> > > +48 696 842 686
> >
> >
> >
> > --
> > mgr inż. Marek Czerski
> > +48 696 842 686
>
> - Marijn
>
> [1]: https://android.googlesource.com/platform/system/bt/+/master/device/include/interop_database.h
--
mgr inż. Marek Czerski
+48 696 842 686
Marijn,
On Wed, Oct 21, 2020 at 3:19 PM Marijn Suijten <[email protected]> wrote:
>
> supported_events was previously initialized based on whatever the AV
> Remote Controller profile running on the peer device could request based
> on its version, and assumes BlueZ is running in the AV Remote Target
> profile.
> If however BlueZ runs the Remote Controller profile (is an audio sink)
> against a Remote Target profile on the peer (the audio source) this
> version is incorrectly taken into account: a Remote Controller profile
> on the peer is not involved in this connection. If its version is too
> low supported_events will not contain all events the peer might
> rightfully attempt to register.
>
> This is particularly problematic with Android phones as an A2DP audio
> source playing back to BlueZ which is the sink. Most Android phones
> report their Remote Controller profile version as 1.3 when initializing
> as audio source [1] meaning that AVRCP_EVENT_VOLUME_CHANGED is
> inadvertently rejected in avrcp_handle_register_notification. As
> mentioned above this profile is of no relevance to the connection, only
> the Remote Target on the phone (source) and Remote Controller on BlueZ
> (sink) take part.
>
> The problem is addressed by splitting supported_events in two variables:
> target_supported_events containing all events the peer Remote Controller
> might attempt to register with the local Remote Target profile, and
> controller_supported_events containing all events the Remote Target
> might attempt to register with the local Remote Controller profile.
>
> In addition the possible notifications going either way have been
> limited. An audio source is in control over media playback and will
> never request playback state changes from the Remote Controller.
> Likewise the audio sink is in control over its rendering volume and will
> never have to request volume notifications from the Remote Target.
> This does however still allow the Remote Controller to send playback
> control messages to the source, and the Remote Target to send
> SetAbsoluteVolume to the sink; both of which are not notifications.
>
> [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#761
>
> ---
>
> Hi Luiz, Marek,
>
> This is a preliminary version of the patch that aims to address the
> issues covered in our mail thread. Keep in mind it is "intentionally"
> messy; I commented out unexpected events based on logically deriving the
> possibilities (as described in the message above) without checking if
> this is in accordance with the documentation.
>
> I still have to test this between two devices that can both run an audio
> sink and source, such as two machines running BlueZ. Playing back audio
> both ways should never make this collapse on its own, though I think at
> that point multiple transports are associated with a device and
> media_transport_is_source would be lying, based on whichever transport
> comes first in the list. Likewise registered_events needs to be split
> in two variables as well.
>
> I'm not sure what's causing the race condition Marek was observing. I
> assume the call to avrcp_get_capabilities or avrcp_connect_browsing in
> controller_init triggers the peer to start requesting capabilities and
> registering for notifications, before target_init is called (which would
> previously be too late to initialize supported_events). That case will
> also be addressed in this patch, but if it happens "at random" because
> the pdu handler is registered before supported_events is assigned I
> propose to split session_init_control in 3 steps instead:
>
> 1. Retrieve remote profile versions and set up *_supported_events;
> 2. Register pdu and passthrough handler;
> 3. The rest from {controller,target}_init.
>
> Looking forward to hearing your suggestions.
>
> Best regards,
> Marijn Suijten
>
> profiles/audio/avrcp.c | 58 +++++++++++++++++++++++++++++++-------
> profiles/audio/transport.c | 20 +++++++++++++
> profiles/audio/transport.h | 1 +
> 3 files changed, 69 insertions(+), 10 deletions(-)
>
> diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> index c093deac8..af5dc4212 100644
> --- a/profiles/audio/avrcp.c
> +++ b/profiles/audio/avrcp.c
> @@ -269,7 +269,13 @@ struct avrcp {
> unsigned int control_id;
> unsigned int browsing_id;
> unsigned int browsing_timer;
> - uint16_t supported_events;
> + // TODO: Swap names to make them represent the name of the peer profile,
Please do not use c++ tyle when commenting use /* */
> + // instead of the opposite local profile?
> + /* Events the Remote Target expects based on peer Remote Controller version */
> + uint16_t target_supported_events;
> + /* Events the Remote Controller expects based on peer Remote Target version */
> + uint16_t controller_supported_events;
> + // TODO: Registered_events should be split across controller/target too!
> uint16_t registered_events;
I'd prefer to have a separate struct:
struct avrcp_role {
uint16_t version;
uint16_t supported_events;
uint16_t registered_events;
};
struct avrcp {
...
struct avrcp_role ct;
struct avrcp_role tg;
> uint8_t transaction;
> uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
> @@ -1060,7 +1066,7 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
> struct avrcp_header *pdu,
> uint8_t transaction)
> {
> - uint16_t len = ntohs(pdu->params_len);
> + uint16_t len = ntohs(pdu->params_len), supported_events;
> unsigned int i;
>
> if (len != 1)
> @@ -1068,6 +1074,11 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
>
> DBG("id=%u", pdu->params[0]);
>
> + if (media_transport_is_source(session->dev))
> + supported_events = session->target_supported_events;
> + else
> + supported_events = session->controller_supported_events;
I guess you did not fully understand my comments regarding AVRCP
roles, the roles are solely based on client/server, so here it is a
server/target no matter the A2DP role since we are receiving a
command.
> switch (pdu->params[0]) {
> case CAP_COMPANY_ID:
> for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> @@ -1082,7 +1093,7 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
> case CAP_EVENTS_SUPPORTED:
> pdu->params[1] = 0;
> for (i = 1; i <= AVRCP_EVENT_LAST; i++) {
> - if (session->supported_events & (1 << i)) {
> + if (supported_events & (1 << i)) {
> pdu->params[1]++;
> pdu->params[pdu->params[1] + 1] = i;
> }
> @@ -1607,7 +1618,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> {
> struct avrcp_player *player = target_get_player(session);
> struct btd_device *dev = session->dev;
> - uint16_t len = ntohs(pdu->params_len);
> + uint16_t len = ntohs(pdu->params_len), supported_events;
> uint64_t uid;
> int8_t volume;
>
> @@ -1620,7 +1631,12 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> goto err;
>
> /* Check if event is supported otherwise reject */
> - if (!(session->supported_events & (1 << pdu->params[0])))
> + if (media_transport_is_source(session->dev))
> + supported_events = session->target_supported_events;
> + else
> + supported_events = session->controller_supported_events;
> +
> + if (!(supported_events & (1 << pdu->params[0])))
> goto err;
Ditto.
> switch (pdu->params[0]) {
> @@ -4129,7 +4145,11 @@ static void target_init(struct avrcp *session)
> media_transport_update_device_volume(session->dev, init_volume);
> }
>
> - session->supported_events |= (1 << AVRCP_EVENT_STATUS_CHANGED) |
> + if (target->version < 0x0103)
> + return;
> +
> + session->target_supported_events |=
> + (1 << AVRCP_EVENT_STATUS_CHANGED) |
> (1 << AVRCP_EVENT_TRACK_CHANGED) |
> (1 << AVRCP_EVENT_TRACK_REACHED_START) |
> (1 << AVRCP_EVENT_TRACK_REACHED_END) |
> @@ -4138,10 +4158,13 @@ static void target_init(struct avrcp *session)
> if (target->version < 0x0104)
> return;
>
> - session->supported_events |=
> + session->target_supported_events |=
> (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
> - (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
> - (1 << AVRCP_EVENT_VOLUME_CHANGED);
> + (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED);
> + // Does not make sense here; the remote is the
> + // rendering device and in control, it'll never
> + // request this notification.
> + // (1 << AVRCP_EVENT_VOLUME_CHANGED);
Again you are think the roles are based on A2DP roles when they are
not, target supported events should reflect what events we support as
server, so we should indicate that we support volume changed even when
acting as a A2DP Source, although this seems to be always omitting
volume changed which would be a regression.
> /* Only check capabilities if controller is not supported */
> if (session->controller == NULL)
> @@ -4180,11 +4203,26 @@ static void controller_init(struct avrcp *session)
> if (controller->version < 0x0103)
> return;
>
> - avrcp_get_capabilities(session);
> + // Players should only run on the remote target; they
> + // should never request notifications about their own
> + // playback status.
> + // session->controller_supported_events |=
> + // (1 << AVRCP_EVENT_STATUS_CHANGED) |
> + // (1 << AVRCP_EVENT_TRACK_CHANGED) |
> + // (1 << AVRCP_EVENT_TRACK_REACHED_START) |
> + // (1 << AVRCP_EVENT_TRACK_REACHED_END) |
> + // (1 << AVRCP_EVENT_SETTINGS_CHANGED);
The controller/client supported_events should reflect what the remote
target/server supports, so here we should probably not initialize with
anything (or perhaps initialize with the mandatory ones if there are
any events that the spec indicates as mandatory) but read the
supported events from the remote with avrcp_get_capabilities.
> if (controller->version < 0x0104)
> return;
>
> + session->controller_supported_events |=
> + // (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
> + // (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
> + (1 << AVRCP_EVENT_VOLUME_CHANGED);
> +
> + avrcp_get_capabilities(session);
> +
> if (!(controller->features & AVRCP_FEATURE_BROWSING))
> return;
>
> diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> index 8248014ae..f5226776f 100644
> --- a/profiles/audio/transport.c
> +++ b/profiles/audio/transport.c
> @@ -980,3 +980,23 @@ void media_transport_update_device_volume(struct btd_device *dev,
> media_transport_update_volume(transport, volume);
> }
> }
> +
> +gboolean media_transport_is_source(struct btd_device *dev)
> +{
> + GSList *l;
> + const char *uuid;
> +
> + if (dev == NULL)
> + return FALSE;
> +
> + for (l = transports; l; l = l->next) {
> + struct media_transport *transport = l->data;
> + if (transport->device != dev)
> + continue;
> +
> + uuid = media_endpoint_get_uuid(transport->endpoint);
> + return strcasecmp(uuid, A2DP_SOURCE_UUID) == 0;
> + }
> +
> + return FALSE;
> +}
> diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
> index 51a67ea74..eb1621813 100644
> --- a/profiles/audio/transport.h
> +++ b/profiles/audio/transport.h
> @@ -30,3 +30,4 @@ void transport_get_properties(struct media_transport *transport,
> int8_t media_transport_get_device_volume(struct btd_device *dev);
> void media_transport_update_device_volume(struct btd_device *dev,
> int8_t volume);
> +gboolean media_transport_is_source(struct btd_device *dev);
> --
> 2.29.0
>
--
Luiz Augusto von Dentz
Luiz,
On Thu, 22 Oct 2020 at 22:11, Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Marijn,
>
> On Wed, Oct 21, 2020 at 3:19 PM Marijn Suijten <[email protected]> wrote:
> >
> > supported_events was previously initialized based on whatever the AV
> > Remote Controller profile running on the peer device could request based
> > on its version, and assumes BlueZ is running in the AV Remote Target
> > profile.
> > If however BlueZ runs the Remote Controller profile (is an audio sink)
> > against a Remote Target profile on the peer (the audio source) this
> > version is incorrectly taken into account: a Remote Controller profile
> > on the peer is not involved in this connection. If its version is too
> > low supported_events will not contain all events the peer might
> > rightfully attempt to register.
> >
> > This is particularly problematic with Android phones as an A2DP audio
> > source playing back to BlueZ which is the sink. Most Android phones
> > report their Remote Controller profile version as 1.3 when initializing
> > as audio source [1] meaning that AVRCP_EVENT_VOLUME_CHANGED is
> > inadvertently rejected in avrcp_handle_register_notification. As
> > mentioned above this profile is of no relevance to the connection, only
> > the Remote Target on the phone (source) and Remote Controller on BlueZ
> > (sink) take part.
> >
> > The problem is addressed by splitting supported_events in two variables:
> > target_supported_events containing all events the peer Remote Controller
> > might attempt to register with the local Remote Target profile, and
> > controller_supported_events containing all events the Remote Target
> > might attempt to register with the local Remote Controller profile.
> >
> > In addition the possible notifications going either way have been
> > limited. An audio source is in control over media playback and will
> > never request playback state changes from the Remote Controller.
> > Likewise the audio sink is in control over its rendering volume and will
> > never have to request volume notifications from the Remote Target.
> > This does however still allow the Remote Controller to send playback
> > control messages to the source, and the Remote Target to send
> > SetAbsoluteVolume to the sink; both of which are not notifications.
> >
> > [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#761
> >
> > ---
> >
> > Hi Luiz, Marek,
> >
> > This is a preliminary version of the patch that aims to address the
> > issues covered in our mail thread. Keep in mind it is "intentionally"
> > messy; I commented out unexpected events based on logically deriving the
> > possibilities (as described in the message above) without checking if
> > this is in accordance with the documentation.
> >
> > I still have to test this between two devices that can both run an audio
> > sink and source, such as two machines running BlueZ. Playing back audio
> > both ways should never make this collapse on its own, though I think at
> > that point multiple transports are associated with a device and
> > media_transport_is_source would be lying, based on whichever transport
> > comes first in the list. Likewise registered_events needs to be split
> > in two variables as well.
> >
> > I'm not sure what's causing the race condition Marek was observing. I
> > assume the call to avrcp_get_capabilities or avrcp_connect_browsing in
> > controller_init triggers the peer to start requesting capabilities and
> > registering for notifications, before target_init is called (which would
> > previously be too late to initialize supported_events). That case will
> > also be addressed in this patch, but if it happens "at random" because
> > the pdu handler is registered before supported_events is assigned I
> > propose to split session_init_control in 3 steps instead:
> >
> > 1. Retrieve remote profile versions and set up *_supported_events;
> > 2. Register pdu and passthrough handler;
> > 3. The rest from {controller,target}_init.
> >
> > Looking forward to hearing your suggestions.
> >
> > Best regards,
> > Marijn Suijten
> >
> > profiles/audio/avrcp.c | 58 +++++++++++++++++++++++++++++++-------
> > profiles/audio/transport.c | 20 +++++++++++++
> > profiles/audio/transport.h | 1 +
> > 3 files changed, 69 insertions(+), 10 deletions(-)
> >
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index c093deac8..af5dc4212 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -269,7 +269,13 @@ struct avrcp {
> > unsigned int control_id;
> > unsigned int browsing_id;
> > unsigned int browsing_timer;
> > - uint16_t supported_events;
> > + // TODO: Swap names to make them represent the name of the peer profile,
>
> Please do not use c++ tyle when commenting use /* */
This is a preliminary patch to aid our discussions and show a
potential solution to the problem. If you want to enforce commenting
style there as well I'll clean that up next time. It wasn't my intent
to get this merged at all anyway.
>
> > + // instead of the opposite local profile?
> > + /* Events the Remote Target expects based on peer Remote Controller version */
> > + uint16_t target_supported_events;
> > + /* Events the Remote Controller expects based on peer Remote Target version */
> > + uint16_t controller_supported_events;
> > + // TODO: Registered_events should be split across controller/target too!
> > uint16_t registered_events;
>
> I'd prefer to have a separate struct:
>
> struct avrcp_role {
> uint16_t version;
> uint16_t supported_events;
> uint16_t registered_events;
> };
Agreed, though I don't know if we need the version. Perhaps this comes
in handy in other places.
> struct avrcp {
> ...
> struct avrcp_role ct;
> struct avrcp_role tg;
CT/TG? I've read section 2.2.1 of the 1.6.2 over and over again, this
doesn't make sense.
>
> > uint8_t transaction;
> > uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
> > @@ -1060,7 +1066,7 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
> > struct avrcp_header *pdu,
> > uint8_t transaction)
> > {
> > - uint16_t len = ntohs(pdu->params_len);
> > + uint16_t len = ntohs(pdu->params_len), supported_events;
> > unsigned int i;
> >
> > if (len != 1)
> > @@ -1068,6 +1074,11 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
> >
> > DBG("id=%u", pdu->params[0]);
> >
> > + if (media_transport_is_source(session->dev))
> > + supported_events = session->target_supported_events;
> > + else
> > + supported_events = session->controller_supported_events;
>
> I guess you did not fully understand my comments regarding AVRCP
> roles, the roles are solely based on client/server, so here it is a
> server/target no matter the A2DP role since we are receiving a
> command.
Indeed, I don't understand this anymore at all. The A2DP role should
map directly to the AV Remote (0x110e) and AV Remote Target (0x110c).
It is my understanding that these define what _direction_ events could
possibly go in. For example in 6.13.2 SetAbsoluteVolume we find:
It is expected that the audio sink will perform as the TG for this command.
Then, in 6.13.3 Notify Volume Change:
This Register Notification event is used by the CT to detect when
the volume has been changed locally on the TG, or what the actual
volume level is following use of relative volume commands.
[...]
It is expected for this command that the audio sink fulfills the TG role.
That matches what I am trying to explain in all these mails. We
cannot assume in this function that we are always the server or TG; if
we are the audio source, we must be the CT (when Absolute Volume is
concerned). This is in accordance with the logical expectation that
the sink is in control of the volume. The target (source) can change
it by sending SetAbsoluteVolume, or ask the controller (sink) to
update it about volume changes by means of registering this event.
Not the other way around (what would happen if the sink starts sending
SetAbsoluteVolume to the source?).
What can we do to clarify the difference between CT/TG,
target/controller (the profiles), source/sink, client/server?
Terminology is going all over the place to the point that I don't know
how to express my understanding of the system (nor specs) anymore.
> > switch (pdu->params[0]) {
> > case CAP_COMPANY_ID:
> > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> > @@ -1082,7 +1093,7 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
> > case CAP_EVENTS_SUPPORTED:
> > pdu->params[1] = 0;
> > for (i = 1; i <= AVRCP_EVENT_LAST; i++) {
> > - if (session->supported_events & (1 << i)) {
> > + if (supported_events & (1 << i)) {
> > pdu->params[1]++;
> > pdu->params[pdu->params[1] + 1] = i;
> > }
> > @@ -1607,7 +1618,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > {
> > struct avrcp_player *player = target_get_player(session);
> > struct btd_device *dev = session->dev;
> > - uint16_t len = ntohs(pdu->params_len);
> > + uint16_t len = ntohs(pdu->params_len), supported_events;
> > uint64_t uid;
> > int8_t volume;
> >
> > @@ -1620,7 +1631,12 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > goto err;
> >
> > /* Check if event is supported otherwise reject */
> > - if (!(session->supported_events & (1 << pdu->params[0])))
> > + if (media_transport_is_source(session->dev))
> > + supported_events = session->target_supported_events;
> > + else
> > + supported_events = session->controller_supported_events;
> > +
> > + if (!(supported_events & (1 << pdu->params[0])))
> > goto err;
>
> Ditto.
How do you propose to figure out what "role" we are providing when
this function is called? As explained above it depends if we are
sink/controller or source/target what events we are expecting to be
registered.
>
> > switch (pdu->params[0]) {
> > @@ -4129,7 +4145,11 @@ static void target_init(struct avrcp *session)
> > media_transport_update_device_volume(session->dev, init_volume);
> > }
> >
> > - session->supported_events |= (1 << AVRCP_EVENT_STATUS_CHANGED) |
> > + if (target->version < 0x0103)
> > + return;
> > +
> > + session->target_supported_events |=
> > + (1 << AVRCP_EVENT_STATUS_CHANGED) |
> > (1 << AVRCP_EVENT_TRACK_CHANGED) |
> > (1 << AVRCP_EVENT_TRACK_REACHED_START) |
> > (1 << AVRCP_EVENT_TRACK_REACHED_END) |
> > @@ -4138,10 +4158,13 @@ static void target_init(struct avrcp *session)
> > if (target->version < 0x0104)
> > return;
> >
> > - session->supported_events |=
> > + session->target_supported_events |=
> > (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
> > - (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
> > - (1 << AVRCP_EVENT_VOLUME_CHANGED);
> > + (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED);
> > + // Does not make sense here; the remote is the
> > + // rendering device and in control, it'll never
> > + // request this notification.
> > + // (1 << AVRCP_EVENT_VOLUME_CHANGED);
>
> Again you are think the roles are based on A2DP roles when they are
> not, target supported events should reflect what events we support as
> server,
I think you are confusing what these variables represent. In the
original BlueZ code this value is based on the version of the peer
AVRCP_REMOTE_UUID. That to me means we are checking whether we are
expecting our peer to register for that particular event, and
rejecting it otherwise. I simply extended this to do the same for the
remote version of the AVRCP_TARGET_UUID profile as well.
What you are suggesting is implicitly encoded as well: if we cannot
handle an event registration even though the peer could realistically
request it based on its version, it's not in the list.
> so we should indicate that we support volume changed even when
> acting as a A2DP Source although this seems to be always omitting
> volume changed which would be a regression.
It does not regress, and was tested with a bluetooth headset to
confirm. This is indeed rejecting EVENT_VOLUME_CHANGED, and for a
good reason as explained above. If BlueZ is the source, _it_ will
register this notification with the headset, not the other way around.
Note that get_capabilities_resp/register_notification do not check
*_supported_events, and they shouldn't.
However, say we flip the roles around and make BlueZ a sink, playing
back from an Android phone. Now avrcp_handle_register_notification
takes controller_supported_events and will happily comply when the
source (the Android phone) registers EVENT_VOLUME_CHANGED.
To make this even more explicit insert some log lines in
get_capabilities_resp: this clearly shows when connecting headphones
as sink it only supports EVENT_VOLUME_CHANGED - when connecting an
Android phone as source it reports a bunch of playback/player related
events but not EVENT_VOLUME_CHANGED.
>
> > /* Only check capabilities if controller is not supported */
> > if (session->controller == NULL)
> > @@ -4180,11 +4203,26 @@ static void controller_init(struct avrcp *session)
> > if (controller->version < 0x0103)
> > return;
> >
> > - avrcp_get_capabilities(session);
> > + // Players should only run on the remote target; they
> > + // should never request notifications about their own
> > + // playback status.
> > + // session->controller_supported_events |=
> > + // (1 << AVRCP_EVENT_STATUS_CHANGED) |
> > + // (1 << AVRCP_EVENT_TRACK_CHANGED) |
> > + // (1 << AVRCP_EVENT_TRACK_REACHED_START) |
> > + // (1 << AVRCP_EVENT_TRACK_REACHED_END) |
> > + // (1 << AVRCP_EVENT_SETTINGS_CHANGED);
>
> The controller/client supported_events should reflect what the remote
> target/server supports, so here we should probably not initialize with
> anything (or perhaps initialize with the mandatory ones if there are
> any events that the spec indicates as mandatory) but read the
> supported events from the remote with avrcp_get_capabilities.
I think we can initialize this list (as well as
target_supported_events) based on what we expect (as explained above),
and AND it as soon as get_capabilities_resp tells us what the remote
can comply with.
> > if (controller->version < 0x0104)
> > return;
> >
> > + session->controller_supported_events |=
> > + // (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
> > + // (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
> > + (1 << AVRCP_EVENT_VOLUME_CHANGED);
> > +
> > + avrcp_get_capabilities(session);
> > +
> > if (!(controller->features & AVRCP_FEATURE_BROWSING))
> > return;
> >
> > diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> > index 8248014ae..f5226776f 100644
> > --- a/profiles/audio/transport.c
> > +++ b/profiles/audio/transport.c
> > @@ -980,3 +980,23 @@ void media_transport_update_device_volume(struct btd_device *dev,
> > media_transport_update_volume(transport, volume);
> > }
> > }
> > +
> > +gboolean media_transport_is_source(struct btd_device *dev)
> > +{
> > + GSList *l;
> > + const char *uuid;
> > +
> > + if (dev == NULL)
> > + return FALSE;
> > +
> > + for (l = transports; l; l = l->next) {
> > + struct media_transport *transport = l->data;
> > + if (transport->device != dev)
> > + continue;
> > +
> > + uuid = media_endpoint_get_uuid(transport->endpoint);
> > + return strcasecmp(uuid, A2DP_SOURCE_UUID) == 0;
> > + }
> > +
> > + return FALSE;
> > +}
> > diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
> > index 51a67ea74..eb1621813 100644
> > --- a/profiles/audio/transport.h
> > +++ b/profiles/audio/transport.h
> > @@ -30,3 +30,4 @@ void transport_get_properties(struct media_transport *transport,
> > int8_t media_transport_get_device_volume(struct btd_device *dev);
> > void media_transport_update_device_volume(struct btd_device *dev,
> > int8_t volume);
> > +gboolean media_transport_is_source(struct btd_device *dev);
> > --
> > 2.29.0
> >
>
>
> --
> Luiz Augusto von Dentz
- Marijn
> > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {Hi Marijn, Luiz,
I tested this patch in a use case where bluez acts as a2dp sink and an
andorid phone as a2dp source.
It is working as expected.
czw., 22 paź 2020 o 23:59 Marijn Suijten <[email protected]> napisał(a):
>
> Luiz,
>
> On Thu, 22 Oct 2020 at 22:11, Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Marijn,
> >
> > On Wed, Oct 21, 2020 at 3:19 PM Marijn Suijten <[email protected]> wrote:
> > >
> > > supported_events was previously initialized based on whatever the AV
> > > Remote Controller profile running on the peer device could request based
> > > on its version, and assumes BlueZ is running in the AV Remote Target
> > > profile.
> > > If however BlueZ runs the Remote Controller profile (is an audio sink)
> > > against a Remote Target profile on the peer (the audio source) this
> > > version is incorrectly taken into account: a Remote Controller profile
> > > on the peer is not involved in this connection. If its version is too
> > > low supported_events will not contain all events the peer might
> > > rightfully attempt to register.
> > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> > >
> > > This is particularly problematic with Android phones as an A2DP audio
> > > source playing back to BlueZ which is the sink. Most Android phones
> > > report their Remote Controller profile version as 1.3 when initializing
> > > as audio source [1] meaning that AVRCP_EVENT_VOLUME_CHANGED is
> > > inadvertently rejected in avrcp_handle_register_notification. As
> > > mentioned above this profile is of no relevance to the connection, only
> > > the Remote Target on the phone (source) and Remote Controller on BlueZ
> > > (sink) take part.
> > >
> > > The problem is addressed by splitting supported_events in two variables:
> > > target_supported_events containing all events the peer Remote Controller
> > > might attempt to register with the local Remote Target profile, and
> > > controller_supported_events containing all events the Remote Target
> > > might attempt to register with the local Remote Controller profile.
> > >
> > > In addition the possible notifications going either way have been
> > > limited. An audio source is in control over media playback and will
> > > never request playback state changes from the Remote Controller.
> > > Likewise the audio sink is in control over its rendering volume and will
> > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> > > never have to request volume notifications from the Remote Target.
> > > This does however still allow the Remote Controller to send playback
> > > control messages to the source, and the Remote Target to send
> > > SetAbsoluteVolume to the sink; both of which are not notifications.
> > >
> > > [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#761
> > >
> > > ---
> > >
> > > Hi Luiz, Marek,
> > >
> > > This is a preliminary version of the patch that aims to address the
> > > issues covered in our mail thread. Keep in mind it is "intentionally"
> > > messy; I commented out unexpected events based on logically deriving the
> > > possibilities (as described in the message above) without checking if
> > > this is in accordance with the documentation.
> > >
> > > I still have to test this between two devices that can both run an audio
> > > sink and source, such as two machines running BlueZ. Playing back audio
> > > both ways should never make this collapse on its own, though I think at
> > > that point multiple transports are associated with a device and
> > > media_transport_is_source would be lying, based on whichever transport
> > > comes first in the list. Likewise registered_events needs to be split
> > > in two variables as well.
> > >
> > > I'm not sure what's causing the race condition Marek was observing. I
> > > assume the call to avrcp_get_capabilities or avrcp_connect_browsing in
> > > controller_init triggers the peer to start requesting capabilities and
> > > registering for notifications, before target_init is called (which would
> > > previously be too late to initialize supported_events). That case will
> > > also be addressed in this patch, but if it happens "at random" because
> > > the pdu handler is registered before supported_events is assigned I
> > > propose to split session_init_control in 3 steps instead:
> > >
> > > 1. Retrieve remote profile versions and set up *_supported_events;
> > > 2. Register pdu and passthrough handler;
> > > 3. The rest from {controller,target}_init.
> > >
> > > Looking forward to hearing your suggestions.
> > >
> > > Best regards,
> > > Marijn Suijten
> > >
> > > profiles/audio/avrcp.c | 58 +++++++++++++++++++++++++++++++-------
> > > profiles/audio/transport.c | 20 +++++++++++++
> > > profiles/audio/transport.h | 1 +
> > > 3 files changed, 69 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > > index c093deac8..af5dc4212 100644
> > > --- a/profiles/audio/avrcp.c
> > > +++ b/profiles/audio/avrcp.c
> > > @@ -269,7 +269,13 @@ struct avrcp {
> > > unsigned int control_id;
> > > unsigned int browsing_id;
> > > unsigned int browsing_timer;
> > > - uint16_t supported_events;
> > > + // TODO: Swap names to make them represent the name of the peer profile,
> >
> > Please do not use c++ tyle when commenting use /* */
>
> This is a preliminary patch to aid our discussions and show a
> potential solution to the problem. If you want to enforce commenting
> style there as well I'll clean that up next time. It wasn't my intent
> to get this merged at all anyway.
>
> >
> > > + // instead of the opposite local profile?
> > > + /* Events the Remote Target expects based on peer Remote Controller version */
> > > + uint16_t target_supported_events;
> > > + /* Events the Remote Controller expects based on peer Remote Target version */
> > > + uint16_t controller_supported_events;
> > > + // TODO: Registered_events should be split across controller/target too!
> > > uint16_t registered_events;
> >
> > I'd prefer to have a separate struct:
> >
> > struct avrcp_role {
> > uint16_t version;
> > uint16_t supported_events;
> > uint16_t registered_events;
> > };
>
> Agreed, though I don't know if we need the version. Perhaps this comes
> in handy in other places.
>
> > struct avrcp {
> > ...
> > struct avrcp_role ct;
> > struct avrcp_role tg;
>
> CT/TG? I've read section 2.2.1 of the 1.6.2 over and over again, this
> doesn't make sense.
>
> >
> > > uint8_t transaction;
> > > uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
> > > @@ -1060,7 +1066,7 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
> > > struct avrcp_header *pdu,
> > > uint8_t transaction)
> > > {
> > > - uint16_t len = ntohs(pdu->params_len);
> > > + uint16_t len = ntohs(pdu->params_len), supported_events;
> > > unsigned int i;
> > >
> > > if (len != 1)
> > > @@ -1068,6 +1074,11 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
> > >
> > > DBG("id=%u", pdu->params[0]);
> > >
> > > + if (media_transport_is_source(session->dev))
> > > + supported_events = session->target_supported_events;
> > > + else
> > > + supported_events = session->controller_supported_events;
> >
> > I guess you did not fully understand my comments regarding AVRCP
> > roles, the roles are solely based on client/server, so here it is a
> > server/target no matter the A2DP role since we are receiving a
> > command.
>
I think that we all have correct understanding and only wording is misleading.
What I believe Marijn wants to express in this piece of code is that:
AVRCP_GET_CAPABILITIES command can be sent only from the peer acting
as a CT role, but local device capabilities are depending on a2dp
role. It means that if the local device is acting an a2dp sink role in
this specific session it should present capabilities that it actually
can handle as a2dp sink which is only the absolute volume. And if the
local device is acting an a2dp source role in this specific session it
should present capabilities for playback control such as play or
pause. I'm assuming here that the scenario where both devices (local
and peer) are acting both a2dp roles (which means that audio is
flowing in both directions) is not realistic and maybe not even
allowed by the specification.
Maybe renaming target_supported_events and controller_supported_events
names for source_supported_events and sink_supported_events would be
more readable. Because the supported events clearly depend on the a2dp
role of the local device (and on the avrcp version of the peer).
> Indeed, I don't understand this anymore at all. The A2DP role should
> map directly to the AV Remote (0x110e) and AV Remote Target (0x110c).
No, where is that written in the specification ? A2DP sink can act
both AVRCP roles.
> It is my understanding that these define what _direction_ events could
> possibly go in.
Yes, "AV Remote (0x110e) and AV Remote Target (0x110c)" define the
direction of the events.
AV Remote (0x110e) is the one that sends commands and receives
notifications and AV Remote Target (0x110c) is the one that receives
commands and sends notifications.
>For example in 6.13.2 SetAbsoluteVolume we find:
>
> It is expected that the audio sink will perform as the TG for this command.
>
Exactly, for playback control a2dp sink plays the CT role, but for
absolute volume it must play TG role. This is exactly proving that the
a2dp sink role does not force avrcp role. The avrcp role depends on
the action (playback control or volume control).
> Then, in 6.13.3 Notify Volume Change:
>
> This Register Notification event is used by the CT to detect when
> the volume has been changed locally on the TG, or what the actual
> volume level is following use of relative volume commands.
> [...]
> It is expected for this command that the audio sink fulfills the TG role.
>
Another proof for duality of AVRCP roles. For playback control local
device act the CT role, and for absolute volume the local device act
TG role.
> That matches what I am trying to explain in all these mails. We
> cannot assume in this function that we are always the server or TG;
If we are in any of the control_handlers (for example in
avrcp_handle_get_capabilities) we for sure are TG (AVRCP target)
because only CT is allowed to send commands. We just don't know if we
are a2dp source or sink.
>if
> we are the audio source, we must be the CT (when Absolute Volume is
> concerned).
When we are the audio source we must implement CT role for controlling
the absolute volume, but that does not mean that in
avrcp_handle_get_capabilities we play this role.
>This is in accordance with the logical expectation that
> the sink is in control of the volume.
Yes, but the sink is not the one that is sending SetAbsoluteVolume
commands, it is the source that is sending those commands.
>The target (source) can change
> it by sending SetAbsoluteVolume
No, target cannot send commands.
>, or ask the controller (sink) to
> update it about volume changes by means of registering this event.
No, target cannot send commands so it cannot register events on controller.
> Not the other way around (what would happen if the sink starts sending
> SetAbsoluteVolume to the source?).
Yes, sink is not supposed to send SetAbsoluteVolume command, but the
source act as a CT role in the context of absolute volume. I guess
that here is the confusion.
> What can we do to clarify the difference between CT/TG,
> target/controller (the profiles), source/sink, client/server?
> Terminology is going all over the place to the point that I don't know
> how to express my understanding of the system (nor specs) anymore.
>
We should stick strictly to specification terminology.
1. target/controller are not profiles, they are roles of the AVRCP
profile. So the profile is only one which has two roles.
2. These roles are represented by the services "AV Remote (0x110e)"
and "AV Remote Target (0x110c)" in the service records. So if one
device is including "AV Remote (0x110e)" service in one of its
service records it means it can act the CT role (but this is not
defining what a2dp role it plays !).
3. source/sink are the roles of the A2DP profile. So there is one A2DP
profile with two roles.
4. client/server - don't use that, there is nothing like that in the
specification.
So bottom line, the way I understand this is that in case when we are
the a2dp sink:
1. if we want to control playback on the a2dp source we must implement
AVRCP CT role for sending playback commands
2, if we want our volume to be controlled by the a2dp source we must
implement AVRCP TG role for receiving commands.
> > > switch (pdu->params[0]) {
> > > case CAP_COMPANY_ID:
> > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> > > @@ -1082,7 +1093,7 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
> > > case CAP_EVENTS_SUPPORTED:
> > > pdu->params[1] = 0;
> > > for (i = 1; i <= AVRCP_EVENT_LAST; i++) {
> > > - if (session->supported_events & (1 << i)) {
> > > + if (supported_events & (1 << i)) {
> > > pdu->params[1]++;
> > > pdu->params[pdu->params[1] + 1] = i;
> > > }
> > > @@ -1607,7 +1618,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > > {
> > > struct avrcp_player *player = target_get_player(session);
> > > struct btd_device *dev = session->dev;
> > > - uint16_t len = ntohs(pdu->params_len);
> > > + uint16_t len = ntohs(pdu->params_len), supported_events;
> > > uint64_t uid;
> > > int8_t volume;
> > >
> > > @@ -1620,7 +1631,12 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > > goto err;
> > >
> > > /* Check if event is supported otherwise reject */
> > > - if (!(session->supported_events & (1 << pdu->params[0])))
> > > + if (media_transport_is_source(session->dev))
> > > + supported_events = session->target_supported_events;
> > > + else
> > > + supported_events = session->controller_supported_events;
> > > +
> > > + if (!(supported_events & (1 << pdu->params[0])))
> > > goto err;
> >
> > Ditto.
>
> How do you propose to figure out what "role" we are providing when
> this function is called? As explained above it depends if we are
> sink/controller or source/target what events we are expecting to be
> registered.
>
Here we are for sure TG because we are receiving a command. This is
the A2DP role which is unknown and on which our capabilities depends.
But I believe that the code here is exactly doing that, it checks if
we are a2dp source or sink.
> >
> > > switch (pdu->params[0]) {
> > > @@ -4129,7 +4145,11 @@ static void target_init(struct avrcp *session)
> > > media_transport_update_device_volume(session->dev, init_volume);
> > > }
> > >
> > > - session->supported_events |= (1 << AVRCP_EVENT_STATUS_CHANGED) |
> > > + if (target->version < 0x0103)
> > > + return;
> > > +
> > > + session->target_supported_events |=
> > > + (1 << AVRCP_EVENT_STATUS_CHANGED) |
> > > (1 << AVRCP_EVENT_TRACK_CHANGED) |
> > > (1 << AVRCP_EVENT_TRACK_REACHED_START) |
> > > (1 << AVRCP_EVENT_TRACK_REACHED_END) |
> > > @@ -4138,10 +4158,13 @@ static void target_init(struct avrcp *session)
> > > if (target->version < 0x0104)
> > > return;
> > >
> > > - session->supported_events |=
> > > + session->target_supported_events |=
> > > (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
> > > - (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
> > > - (1 << AVRCP_EVENT_VOLUME_CHANGED);
> > > + (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED);
> > > + // Does not make sense here; the remote is the
> > > + // rendering device and in control, it'll never
> > > + // request this notification.
> > > + // (1 << AVRCP_EVENT_VOLUME_CHANGED);
> >
> > Again you are think the roles are based on A2DP roles when they are
> > not, target supported events should reflect what events we support as
> > server,
>
> I think you are confusing what these variables represent. In the
> original BlueZ code this value is based on the version of the peer
> AVRCP_REMOTE_UUID. That to me means we are checking whether we are
> expecting our peer to register for that particular event, and
> rejecting it otherwise. I simply extended this to do the same for the
> remote version of the AVRCP_TARGET_UUID profile as well.
> What you are suggesting is implicitly encoded as well: if we cannot
> handle an event registration even though the peer could realistically
> request it based on its version, it's not in the list.
>
supported_events are used only when receiving commands
(get_capabilietes or register_event) and while receiving those
commands we play the TG role. So the peer act as CT so it should
declare AVRCP_REMOTE_UUID(0x110e) service. So only if the peer has
AVRCP_REMOTE_UUID declared supported_events should be initialised. If
we are the a2dp source we should declare that we support playback
control events. If we are a2dp sink we should declare that we support
absolute volume event.
So what I believe Luiz is saying is that both supported_events fields
(target and controller or better name then source and sink) should be
initialized in target_init.
But then we have the problem that peer controller wants to register
absolute volume event before target_init is called.
I think that those supported_events should be statically initialised
separately for a2dp sink and source roles. I cannot understand the
need for checking the peer side avrcp version, is it against the
specification if we declare events that are not supported by the peer
version ? If the peer has avrcp version 1.3 and sends registration
command of absolute volume event then the peer is not consistent with
the specification already.
> > so we should indicate that we support volume changed even when
> > acting as a A2DP Source although this seems to be always omitting
> > volume changed which would be a regression.
>
> It does not regress, and was tested with a bluetooth headset to
> confirm. This is indeed rejecting EVENT_VOLUME_CHANGED, and for a
> good reason as explained above. If BlueZ is the source, _it_ will
> register this notification with the headset, not the other way around.
> Note that get_capabilities_resp/register_notification do not check
> *_supported_events, and they shouldn't.
>
> However, say we flip the roles around and make BlueZ a sink, playing
> back from an Android phone. Now avrcp_handle_register_notification
> takes controller_supported_events and will happily comply when the
> source (the Android phone) registers EVENT_VOLUME_CHANGED.
>
> To make this even more explicit insert some log lines in
> get_capabilities_resp: this clearly shows when connecting headphones
> as sink it only supports EVENT_VOLUME_CHANGED - when connecting an
> Android phone as source it reports a bunch of playback/player related
> events but not EVENT_VOLUME_CHANGED.
>
> >
> > > /* Only check capabilities if controller is not supported */
> > > if (session->controller == NULL)
> > > @@ -4180,11 +4203,26 @@ static void controller_init(struct avrcp *session)
> > > if (controller->version < 0x0103)
> > > return;
> > >
> > > - avrcp_get_capabilities(session);
> > > + // Players should only run on the remote target; they
> > > + // should never request notifications about their own
> > > + // playback status.
> > > + // session->controller_supported_events |=
> > > + // (1 << AVRCP_EVENT_STATUS_CHANGED) |
> > > + // (1 << AVRCP_EVENT_TRACK_CHANGED) |
> > > + // (1 << AVRCP_EVENT_TRACK_REACHED_START) |
> > > + // (1 << AVRCP_EVENT_TRACK_REACHED_END) |
> > > + // (1 << AVRCP_EVENT_SETTINGS_CHANGED);
> >
> > The controller/client supported_events should reflect what the remote
> > target/server supports, so here we should probably not initialize with
> > anything (or perhaps initialize with the mandatory ones if there are
> > any events that the spec indicates as mandatory) but read the
> > supported events from the remote with avrcp_get_capabilities.
>
> I think we can initialize this list (as well as
> target_supported_events) based on what we expect (as explained above),
> and AND it as soon as get_capabilities_resp tells us what the remote
> can comply with.
>
> > > if (controller->version < 0x0104)
> > > return;
> > >
> > > + session->controller_supported_events |=
> > > + // (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
> > > + // (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
> > > + (1 << AVRCP_EVENT_VOLUME_CHANGED);
> > > +
> > > + avrcp_get_capabilities(session);
> > > +
> > > if (!(controller->features & AVRCP_FEATURE_BROWSING))
> > > return;
> > >
> > > diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> > > index 8248014ae..f5226776f 100644
> > > --- a/profiles/audio/transport.c
> > > +++ b/profiles/audio/transport.c
> > > @@ -980,3 +980,23 @@ void media_transport_update_device_volume(struct btd_device *dev,
> > > media_transport_update_volume(transport, volume);
> > > }
> > > }
> > > +
> > > +gboolean media_transport_is_source(struct btd_device *dev)
> > > +{
> > > + GSList *l;
> > > + const char *uuid;
> > > +
> > > + if (dev == NULL)
> > > + return FALSE;
> > > +
> > > + for (l = transports; l; l = l->next) {
> > > + struct media_transport *transport = l->data;
> > > + if (transport->device != dev)
> > > + continue;
> > > +
> > > + uuid = media_endpoint_get_uuid(transport->endpoint);
> > > + return strcasecmp(uuid, A2DP_SOURCE_UUID) == 0;
> > > + }
> > > +
> > > + return FALSE;
> > > +}
> > > diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
> > > index 51a67ea74..eb1621813 100644
> > > --- a/profiles/audio/transport.h
> > > +++ b/profiles/audio/transport.h
> > > @@ -30,3 +30,4 @@ void transport_get_properties(struct media_transport *transport,
> > > int8_t media_transport_get_device_volume(struct btd_device *dev);
> > > void media_transport_update_device_volume(struct btd_device *dev,
> > > int8_t volume);
> > > +gboolean media_transport_is_source(struct btd_device *dev);
> > > --
> > > 2.29.0
> > >
> >
> >
> > --
> > Luiz Augusto von Dentz
>
> - Marijn
--
mgr inż. Marek Czerski
+48 696 842 686
Hi Marek,
On Thu, 22 Oct 2020 at 15:06, Marek Czerski <[email protected]> wrote:
>
> Hi Marijn,
>
> czw., 22 paź 2020 o 00:10 Marijn Suijten <[email protected]> napisał(a):
> >
> > Hi Marek,
> >
> > On Wed, 21 Oct 2020 at 16:02, Marek Czerski <[email protected]> wrote:
> > >
> > > Hi Marijn,
> > >
> > > Regarding the AVRCP version reported by the phone. There is something
> > > weird going on on android regarding avrcp version and mac addresses
> > > and absolute volume. I discovered couple of whings:
> > > 1. galaxy S7 will blacklist some mac addresses to not use absolute
> > > volume for that device (checked it with "adb shell dumpsys
> > > bluetooth_manager"). Changing mac addres of the device to something
> > > other allows for absolute volume to work. For both cases it reports
> > > avrcp version 1.4.
> >
> > There is indeed a database with traits based on mac-addresses [1].
> >
> > > 2. xiaomi redmi note 4 will report different avrcp version depending
> > > on mac address. version 1.3 for mac address that galaxy s7 blacklists
> > > and 1.6 for mac address that galaxy s7 does not blacklist. But in both
> > > cases the absolute volume does not work ...
> >
> > You confirmed earlier that besides this versioning issue there's also
> > an initialization issue with supported_events. I assume this test was
> > without flipping the _init calls around?
> >
>
> To tests if absolute volume is working on redmi note 4 I used two
> different a2dp sink devices, yamaha rx-v485 av receiver and sony
> wh-ch510 headphones. The AVRCP version was read from "sdptool browse"
> output running on my linux machine.
> So in this case, the absolute volume was not working at all on this
> phone despite the fact that it should (developer options were set to
> enable absolute volume). Also the absolute volume on this phone was
> not working with bluez no matter what the order of init calls.
That's odd, though searching for it shows this is a common problem
across Xiaomi products. You might want to manually check getprop
persist.bluetooth.disableabsvol and make sure the phone is rebooted
after enabling absolute volume. Otherwise look around in the Android
logs, they are pretty verbose about absolute volume support.
> > > 3. xiaomi redmi 8 will not even report that it has "A/V_RemoteControl"
> > > (UUID 0x110E), it just reports that it has only
> > > "A/V_RemoteControlTarget" (UUID 0x110C) but still it can control
> > > volume on my yamaha av receiver and sony headphones and receive volume
> > > updates from them. Very, very wierd.
> >
> > That's not really surprising to me, in fact it is slightly weird that
> > other phones report the A/V_RemoteControl profile. A phone will always
> > be the audio source (thus the RemoteControlTarget), never the sink.
> >
>
> I know You were explaining that already but for me it is still a mystery ...
> In AVRCP specification v1.6.2 (this is the newest one on the bluetooth
> sig site) in paragraph 2.2.1 (Roles) there is:
> "The controller (CT) is a device that initiates a transaction by
> sending a command frame to a target."
> So if the device wants to send a command it must play the controller
> role. And SetAbsoluteVolume is the command that is sent from the phone
> when you press volume up/down on the phone. So To support absolute
> volume the phone has to be a controller (and the target to handle
> play/pause etc. commands).
> Next in the same specification in chapter 8 (Service discovery
> interoperability requirements) there is:
> "This profile defines the following service records for the CT and the
> TG, respectively." and below is definition of those records.
> So I understand it that if phone wants to send SetAbsoluteVolume
> command it must implement the controller role (CT) and define the
> service record with services "A/V Remote ControlI" (0x110E) and "A/V
> Remote Control Controller" (0x110F) in "Service Class ID List" field.
Finally, the misunderstanding on my side has been cleared up in
chapter 8. After reading chapter 2.2.1 I blindly assumed the CT and
TG designation were solely indicating message initiator, while
expecting the Remote Control and Remote Target to be analogous to sink
and source respectively. That turns out to not be true at all, CT
role == Remote Control service, TG == Remote Target service.
I honestly wasn't expecting this. It made too much sense that the
target is where the media player lives, while the controller is where
the "remote controls" are (and usually the rendering device). That
way headphones would for example not need to expose the "Remote
Control Target" profile, but this finally explains why my headphones
expose that service.
In other words, in order to have any sort of meaningful communication
between two devices (SetAbsoluteVolume from source to sink, and media
controls from sink to source) both peers should handle both services.
This unfortunately means I need to rewrite the patch again and rectify
all earlier conversations in the patch discussion going on
concurrently.
> And in case of redmi 8, this phone does not define such service record
> and still it sends SetAbsoluteVolume commands.
>
> So either this phone is not respecting the specification or I totally
> missed the point. (I guess the second ...)
>
> > (Unless applying some hacks to libfluoride: it is possible to use an
> > older phone as A2DP sink - for example to connect to a receiver at the
> > other side of the room. I would have enabled it on all my phones if it
> > didn't break A2DP source capabilities at the same time... Maybe
> > Gabeldorsche addresses that)
> >
> > > For the record, to check the avrcp version of the phone I use sdptool
> > > browse command (and to be 100% sure I check also the raw communication
> > > on wireshark). For example this is one of the records reported for
> > > xiaomi redmi note 4 and avrcp version 1.6:
> > > Service RecHandle: 0x10007
> > > Service Class ID List:
> > > "AV Remote" (0x110e)
> > > "AV Remote Controller" (0x110f)
> > > Protocol Descriptor List:
> > > "L2CAP" (0x0100)
> > > PSM: 23
> > > "AVCTP" (0x0017)
> > > uint16: 0x0104
> > > Profile Descriptor List:
> > > "AV Remote" (0x110e)
> > > Version: 0x0106
> > > If there is something more I can do to help please let me know. For
> > > example I can send hci snoop logs for specific cases etc.
> >
> > I will send a _preliminary_ patch to this thread that should resolve
> > both the versioning and ordering issue as I've finally figured a way
> > to check whether the session associated with an AVRCP command is
> > running as source or sink. It is not finalized yet so please don't be
> > too hard on it :)
> >
>
> I will be happy to test it :)
The patch was intended to end up in this mail thread but ended up as a
separate message despite setting in-reply-to correctly. Am I not
allowed to send in-reply-to on my own message, with the to: field not
matching the original sender of that mail? Or is it because the
subject does not match this mail thread? Apologies either way, this
is one of my first dealings with mailing lists :)
> > > wt., 20 paź 2020 o 15:12 Marek Czerski <[email protected]> napisał(a):
> > > >
> > > > Hi Marijn,
> > > >
> > > > wt., 20 paź 2020 o 11:11 Marijn Suijten <[email protected]> napisał(a):
> > > > >
> > > > > Hi Marek,
> > > > >
> > > > > (Apologies for sending this to you twice; missed the reply-all button
> > > > > so this didn't end up on the mailing list)
> > > > >
> > > > > On Tue, 20 Oct 2020 at 09:31, Marek Czerski <[email protected]> wrote:
> > > > > >
> > > > > > Hi Marijn,
> > > > > >
> > > > > > pon., 19 paź 2020 o 21:53 Marijn Suijten <[email protected]> napisał(a):
> > > > > > >
> > > > > > > Hi Luiz, Marek,
> > > > > > >
> > > > > > > On Mon, 19 Oct 2020 at 18:47, Luiz Augusto von Dentz
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > > Hi Marek, Marijn,
> > > > > > > >
> > > > > > > > On Mon, Oct 19, 2020 at 9:06 AM Marek Czerski <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > Hi Marijn,
> > > > > > > > >
> > > > > > > > > pon., 19 paź 2020 o 16:16 Marijn Suijten <[email protected]> napisał(a):
> > > > > > > > > >
> > > > > > > > > > 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
> > > > > > > > > > [email protected] [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.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > I didn't tested Your patch but after looking at the code it seems that
> > > > > > > > > just applying Your patch would solve my problem.
> > > > > > >
> > > > > > > It should, even though I don't immediately see how swapping
> > > > > > > target/controller_init as per your initial email solves the problem.
> > > > > > >
> > > > > >
> > > > > > The sequence of events is important here. The
> > > > > > session->supported_events must be initialised (which is done in
> > > > > > target_init) before first AVRCP_REGISTER_NOTIFICATION command from the
> > > > > > remote side controller. If supported_events is not initialised and
> > > > > > AVRCP_REGISTER_NOTIFICATION arrives the
> > > > > > avrcp_handle_register_notification function rejects the event
> > > > > > registration. And this is what I see in wireshark.
> > > > >
> > > > > That's interesting and means we are dealing with two distinct problems
> > > > > here. No matter what order I flip controller/target_init in
> > > > > AVRCP_REGISTER_NOTIFICATION is always received after both functions
> > > > > complete. handle_vendordep_pdu is registered before these
> > > > > initialization functions making this possible though. I wonder if PDUs
> > > > > are dropped when the callback is registered later, after init. Perhaps
> > > > > controller/target_init need to be split in two, a static
> > > > > initialization part based on the remote profile version followed by
> > > > > registering this callback and finally functions that send commands to
> > > > > the remote like avrcp_get_capabilities.
> > > > >
> > > > > > And in my case, calling target_init earlier (the is before
> > > > > > controller_init) solves the problem because supported_events is
> > > > > > already initialised at the time the AVRCP_REGISTER_NOTIFICATION
> > > > > > arrives.
> > > > >
> > > > > That means your phone reports an AVRCP remote version of at least
> > > > > 0x104, otherwise supported_events would not contain
> > > > > AVRCP_EVENT_VOLUME_CHANGED. Can you validate this in wireshark or
> > > > > DBG("%p version 0x%04x", ...) in the logs? I'm seeing:
> > > > >
> > > > > profiles/audio/avrcp.c:target_init() 0x5614a2cbab70 version 0x0103
> > > > >
> > > >
> > > > Yes, I get this in my logs when connecting from samsung galaxy S7:
> > > > profiles/audio/avrcp.c:target_init() 0x1103758 version 0x0104
> > > > profiles/audio/avrcp.c:controller_init() 0x1112c18 version 0x0104
> > > >
> > > > > Either way the split Luiz and I mentioned mean one variant of
> > > > > supported_events (the one relevant for registering
> > > > > AVRCP_EVENT_VOLUME_CHANGED) is initialized in controller_init.
> > > > >
> > > > > > But I think that the problem is more general. There is some gap
> > > > > > between AVCTP connection establishment and BlueZ readiness to handle
> > > > > > commands from the remote side. There could be devices that send
> > > > > > commands during this gap and those devices would not work correctly
> > > > > > with BlueZ despite the fact that they are consistent with the
> > > > > > specification (or not ?).
> > > > > >
> > > > > > > > > Regarding avrcp version, in android there is developer option to set
> > > > > > > > > avrcp version. For example my Xiaomi redmi 8 (android 10) reports
> > > > > > > > > version according to this setting, but samsung galaxy s7 (android 8)
> > > > > > > > > always report version 1.4 regardless of this setting.
> > > > > > >
> > > > > > > I think this is the version used for the AVRCP Remote Target [1]. The
> > > > > > > version linked above, the one that is causing problems here is that of
> > > > > > > the AVRCP controller.
> > > > > > >
> > > > > > > There have always been problems changing these settings in developer
> > > > > > > options - sometimes they work, sometimes they don't. The system has
> > > > > > > been overhauled for Android 11 and is now much more consistent though
> > > > > > > avrcp still runs through a property. If I remember correctly
> > > > > > > restarting bluetooth or re-adding a device (in case of SDP caching) is
> > > > > > > the only solution.
> > > > > > >
> > > > > > > > We need to rework a little bit how the controller/target works, this
> > > > > > > > roles are actually supposed to be interpreted as client/server and
> > > > > > > > much like GATT they can be used simultaneously, so we need a target
> > > > > > > > versions and a controller version and independent supported events.
> > > > > > >
> > > > > > > Yes, I think that's the change we proposed last time and are proposing
> > > > > > > now. Unfortunately relevant functions need to know if the local
> > > > > > > (BlueZ) end is operating as target or controller for that particular
> > > > > > > operation.
> > > > > > >
> > > > > > > > That said if the remote side controller version does not indicate 1.4
> > > > > > > > or later we obviously can't support absolute volume control as that is
> > > > > > > > reserved in earlier versions.
> > > > > > >
> > > > > > > That depends on the way audio is flowing. When the local (BlueZ) end
> > > > > > > is the sink (rendering the audio) meaning it takes the role of AVRCP
> > > > > > > controller (the situation in the mail from Marek), the remote must be
> > > > > > > the AVRCP target and hence its controller version is irrelevant.
> > > > > > > Likewise when BlueZ is the audio source (AVRCP target), it should only
> > > > > > > care about the remote controller profile and version.
> > > > > > > But that is exactly what you described above: BlueZ needs to attain
> > > > > > > separation between handling notification registrations (and perhaps
> > > > > > > other code) as a controller and target separately.
> > > > > > >
> > > > > >
> > > > > > I don't know if I'm interpreting your words correctly but I think that
> > > > > > You make an assumption that a2dp sink must be avrcp CT and a2dp source
> > > > > > is avrcp TG.
> > > > > > In case of the absolute volume control it is not true. For absolute
> > > > > > volume control to work it is the other way around which makes both
> > > > > > sides be CT and TG at the same time.
> > > > >
> > > > > Quite the contrary, in my previous mail I attempted to explain
> > > > > multiple times that the CT/TG designation in the spec is used for
> > > > > initiator and target respectively, not the AVRCP controller and target
> > > > > profile. Indeed, depending on the transaction either side (controller
> > > > > or target) can be the initiator (CT). In other words CT and TG are
> > > > > _not_ abbreviations for the controller and target profile.
> > > > >
> > > > > > a2dp sink is avrcp CT for
> > > > > > controlling playback and a2dp source is avrcp CT for setting the
> > > > > > volume. It is the a2dp source that is sending
> > > > > > AVRCP_SET_ABSOLUTE_VOLUME commands.
> > > > >
> > > > > Correct, yet the A2DP sink will always take on the AVRCP controller
> > > > > profile where the phone being the A2DP source takes the AVRCP target
> > > > > profile.
> > > > >
> > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > One thing is not clear for me, what is the purpose of the
> > > > > > > > > supported_events ? It is used in two places:
> > > > > > > > > First is the avrcp_handle_register_notification function. If the
> > > > > > > > > remote side want to register itself for specific event notification it
> > > > > > > > > does not matter what version of avrcp that remote side supports. If it
> > > > > > > > > ask for specific event it clearly support that event.
> > > > > > >
> > > > > > > Looking at commit 4ae6135 that introduced this check it was apparently
> > > > > > > causing crashes without. I can only guess that devices were sending
> > > > > > > vendor-specific notification requests, assuming implementations are
> > > > > > > supposed to filter out anything beyond what their reported version
> > > > > > > supports? Either way input validation is still a sane thing to do.
> > > > > > >
> > > > > > > > > Second is in avrcp_handle_get_capabilities in CAP_EVENTS_SUPPORTED
> > > > > > > > > case. Does it matter if local side reply with events that are not
> > > > > > > > > supported in the version of avrcp supported by the remote side ?
> > > > > > >
> > > > > > > I guess it is sane here to not report capabilities the CT should -
> > > > > > > given it's version - not know about. I could not find anything in the
> > > > > > > 1.6.2 spec mandating this though, perhaps Luiz knows.
> > > > > > >
> > > > > > > > As I said we need to split the supported events to
> > > > > > > > ct(client)/tg(server) to avoid interpreting them as the same, it is
> > > > > > > > very odd that the remote would have different versions for each role
> > > > > > >
> > > > > > > Yeah, Android exposes different versions for its AVRCP Remote and
> > > > > > > AVRCP Remote Target.
> > > > > > > This is slightly confusing though: we discussed earlier that CT and TG
> > > > > > > is about the initiator and receiver respectively, with no direct
> > > > > > > mapping to AVRCP remote and target as either side can be the initiator
> > > > > > > (sender) of a command. Correct?
> > > > > > > In this specific case a notification registration will always be
> > > > > > > initiated by a CT and fulfilled by the TG, no matter whether it is an
> > > > > > > AVRCP remote or target.
> > > > > > >
> > > > > > > > but it looks like this is happening in this case although it is work
> > > > > > > > confirming if the CT version if in fact 1.3 as well we cannot enable
> > > > > > > > absolute volume control as that is not supported by that version, what
> > > > > > > > we can perhaps is to detect if SetAbsoluteVolume control is used then
> > > > > > > > update the events for the session.
> > > > > > >
> > > > > > > Again, in the specific case of this issue it is fine if the CT
> > > > > > > (initiator, the Android phone, supposed to behave like the target as
> > > > > > > it is the audio source) reports a controller version of 1.3 as we are
> > > > > > > only concerned about the target version, which will be the one
> > > > > > > registering for EVENT_VOLUME_CHANGED notifications from the
> > > > > > > sink/controller (BlueZ). Android always reports the AVRCP Remote
> > > > > > > Target as 1.4 or higher based on developer settings [1].
> > > > > > >
> > > > > > > > > > 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
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > Best regards,
> > > > > > > > > Marek Czerski
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > Luiz Augusto von Dentz
> > > > > > >
> > > > > > > P.S.: I hope it is fine to respond to two emails in one, seems like
> > > > > > > that will get confusing real quick if depth increases.
> > > > > > >
> > > > > > > - Marijn
> > > > > > >
> > > > > > > [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#612
> > > > > >
> > > > > > Best regards,
> > > > > > Marek
> > > > >
> > > > > - Marijn
> > > >
> > > >
> > > >
> > > > --
> > > > mgr inż. Marek Czerski
> > > > +48 696 842 686
> > >
> > >
> > >
> > > --
> > > mgr inż. Marek Czerski
> > > +48 696 842 686
> >
> > - Marijn
> >
> > [1]: https://android.googlesource.com/platform/system/bt/+/master/device/include/interop_database.h
>
>
>
> --
> mgr inż. Marek Czerski
> +48 696 842 686
Thanks for clearing up my stubbornness in not seeing the relation
between CT/TG and the Remote Control/Remote Control Target service!
- Marijn
Hi Luiz, Marek,
I have to apologize upfront for prioritizing replying to Luiz' review
of this patch before checking Marek's message in the other mail
thread. As it turns out my expectation that CT/TG are orthogonal from
the Remote Control and Remote Control Target service is completely
wrong; they are in fact analogous. That explains all the
misunderstandings thus far. I will revise this patch and resend it
with the new insights I have right now.
This means that the code in BlueZ currently is correct.
AVRCP_REGISTER_NOTIFICATION will only ever be sent from CT to TG,
supported_events can thus only be defined by whatever the CT service
supports (and implicitly encodes what BlueZ as TG supports; if it is
not supported, the code to add the event to supported_events simply
won't be there). I have unfortunately lost and cannot find the
specification for older AVRCP versions anymore, but the 1.5 and 1.6.2
spec mention nothing about requiring at least version 0x104 for
Absolute Volume. This is instead unambiguously clarified in "4.5
AVRCP specific commands": the Absolute Volume event and notification
are mandatory when CATEGORY_2 is reported. For what it's worth I
think the sink/source distinction I tried to make in previous mails is
instead defined by these categories.
Luiz what do you reckon: is it a quirk of Android to report version
0x103 while also reporting CATEGORY_2, and is it okay to solve this by
removing the version check and enable EVENT_VOLUME_CHANGED when the C2
feature is reported by the CT?
Also, sending get_capabilities from target_init doesn't make much
sense. If session->controller is NULL that means the remote doesn't
have the TG profile and thus can't handle the command?
Marek, I'll omit replying to most of your points after understanding
this finally, for convenience.
On Fri, 23 Oct 2020 at 10:43, Marek Czerski <[email protected]> wrote:
>
> > > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> > > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {Hi Marijn, Luiz,
>
> I tested this patch in a use case where bluez acts as a2dp sink and an
> andorid phone as a2dp source.
> It is working as expected.
>
> czw., 22 paź 2020 o 23:59 Marijn Suijten <[email protected]> napisał(a):
> >
> > Luiz,
> >
> > On Thu, 22 Oct 2020 at 22:11, Luiz Augusto von Dentz
> > <[email protected]> wrote:
> > >
> > > Marijn,
> > >
> > > On Wed, Oct 21, 2020 at 3:19 PM Marijn Suijten <[email protected]> wrote:
> > > >
> > > > supported_events was previously initialized based on whatever the AV
> > > > Remote Controller profile running on the peer device could request based
> > > > on its version, and assumes BlueZ is running in the AV Remote Target
> > > > profile.
> > > > If however BlueZ runs the Remote Controller profile (is an audio sink)
> > > > against a Remote Target profile on the peer (the audio source) this
> > > > version is incorrectly taken into account: a Remote Controller profile
> > > > on the peer is not involved in this connection. If its version is too
> > > > low supported_events will not contain all events the peer might
> > > > rightfully attempt to register.
>
> > > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
>
> > > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> > > >
> > > > This is particularly problematic with Android phones as an A2DP audio
> > > > source playing back to BlueZ which is the sink. Most Android phones
> > > > report their Remote Controller profile version as 1.3 when initializing
> > > > as audio source [1] meaning that AVRCP_EVENT_VOLUME_CHANGED is
> > > > inadvertently rejected in avrcp_handle_register_notification. As
> > > > mentioned above this profile is of no relevance to the connection, only
> > > > the Remote Target on the phone (source) and Remote Controller on BlueZ
> > > > (sink) take part.
> > > >
> > > > The problem is addressed by splitting supported_events in two variables:
> > > > target_supported_events containing all events the peer Remote Controller
> > > > might attempt to register with the local Remote Target profile, and
> > > > controller_supported_events containing all events the Remote Target
> > > > might attempt to register with the local Remote Controller profile.
> > > >
> > > > In addition the possible notifications going either way have been
> > > > limited. An audio source is in control over media playback and will
> > > > never request playback state changes from the Remote Controller.
> > > > Likewise the audio sink is in control over its rendering volume and will
>
> > > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> > > > never have to request volume notifications from the Remote Target.
> > > > This does however still allow the Remote Controller to send playback
> > > > control messages to the source, and the Remote Target to send
> > > > SetAbsoluteVolume to the sink; both of which are not notifications.
> > > >
> > > > [1]: https://android.googlesource.com/platform/system/bt/+/android-11.0.0_r4/bta/av/bta_av_main.cc#761
> > > >
> > > > ---
> > > >
> > > > Hi Luiz, Marek,
> > > >
> > > > This is a preliminary version of the patch that aims to address the
> > > > issues covered in our mail thread. Keep in mind it is "intentionally"
> > > > messy; I commented out unexpected events based on logically deriving the
> > > > possibilities (as described in the message above) without checking if
> > > > this is in accordance with the documentation.
> > > >
> > > > I still have to test this between two devices that can both run an audio
> > > > sink and source, such as two machines running BlueZ. Playing back audio
> > > > both ways should never make this collapse on its own, though I think at
> > > > that point multiple transports are associated with a device and
> > > > media_transport_is_source would be lying, based on whichever transport
> > > > comes first in the list. Likewise registered_events needs to be split
> > > > in two variables as well.
> > > >
> > > > I'm not sure what's causing the race condition Marek was observing. I
> > > > assume the call to avrcp_get_capabilities or avrcp_connect_browsing in
> > > > controller_init triggers the peer to start requesting capabilities and
> > > > registering for notifications, before target_init is called (which would
> > > > previously be too late to initialize supported_events). That case will
> > > > also be addressed in this patch, but if it happens "at random" because
> > > > the pdu handler is registered before supported_events is assigned I
> > > > propose to split session_init_control in 3 steps instead:
> > > >
> > > > 1. Retrieve remote profile versions and set up *_supported_events;
> > > > 2. Register pdu and passthrough handler;
> > > > 3. The rest from {controller,target}_init.
> > > >
> > > > Looking forward to hearing your suggestions.
> > > >
> > > > Best regards,
> > > > Marijn Suijten
> > > >
> > > > profiles/audio/avrcp.c | 58 +++++++++++++++++++++++++++++++-------
> > > > profiles/audio/transport.c | 20 +++++++++++++
> > > > profiles/audio/transport.h | 1 +
> > > > 3 files changed, 69 insertions(+), 10 deletions(-)
> > > >
> > > > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > > > index c093deac8..af5dc4212 100644
> > > > --- a/profiles/audio/avrcp.c
> > > > +++ b/profiles/audio/avrcp.c
> > > > @@ -269,7 +269,13 @@ struct avrcp {
> > > > unsigned int control_id;
> > > > unsigned int browsing_id;
> > > > unsigned int browsing_timer;
> > > > - uint16_t supported_events;
> > > > + // TODO: Swap names to make them represent the name of the peer profile,
> > >
> > > Please do not use c++ tyle when commenting use /* */
> >
> > This is a preliminary patch to aid our discussions and show a
> > potential solution to the problem. If you want to enforce commenting
> > style there as well I'll clean that up next time. It wasn't my intent
> > to get this merged at all anyway.
> >
> > >
> > > > + // instead of the opposite local profile?
> > > > + /* Events the Remote Target expects based on peer Remote Controller version */
> > > > + uint16_t target_supported_events;
> > > > + /* Events the Remote Controller expects based on peer Remote Target version */
> > > > + uint16_t controller_supported_events;
> > > > + // TODO: Registered_events should be split across controller/target too!
> > > > uint16_t registered_events;
> > >
> > > I'd prefer to have a separate struct:
> > >
> > > struct avrcp_role {
> > > uint16_t version;
> > > uint16_t supported_events;
> > > uint16_t registered_events;
> > > };
> >
> > Agreed, though I don't know if we need the version. Perhaps this comes
> > in handy in other places.
> >
> > > struct avrcp {
> > > ...
> > > struct avrcp_role ct;
> > > struct avrcp_role tg;
> >
> > CT/TG? I've read section 2.2.1 of the 1.6.2 over and over again, this
> > doesn't make sense.
It makes sense now: we don't need to separate out *_events between CT
and TG at all.
> >
> > >
> > > > uint8_t transaction;
> > > > uint8_t transaction_events[AVRCP_EVENT_LAST + 1];
> > > > @@ -1060,7 +1066,7 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
> > > > struct avrcp_header *pdu,
> > > > uint8_t transaction)
> > > > {
> > > > - uint16_t len = ntohs(pdu->params_len);
> > > > + uint16_t len = ntohs(pdu->params_len), supported_events;
> > > > unsigned int i;
> > > >
> > > > if (len != 1)
> > > > @@ -1068,6 +1074,11 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
> > > >
> > > > DBG("id=%u", pdu->params[0]);
> > > >
> > > > + if (media_transport_is_source(session->dev))
> > > > + supported_events = session->target_supported_events;
> > > > + else
> > > > + supported_events = session->controller_supported_events;
> > >
> > > I guess you did not fully understand my comments regarding AVRCP
> > > roles, the roles are solely based on client/server, so here it is a
> > > server/target no matter the A2DP role since we are receiving a
> > > command.
> >
>
> I think that we all have correct understanding and only wording is misleading.
> What I believe Marijn wants to express in this piece of code is that:
> AVRCP_GET_CAPABILITIES command can be sent only from the peer acting
> as a CT role, but local device capabilities are depending on a2dp
> role. It means that if the local device is acting an a2dp sink role in
> this specific session it should present capabilities that it actually
> can handle as a2dp sink which is only the absolute volume. And if the
> local device is acting an a2dp source role in this specific session it
> should present capabilities for playback control such as play or
> pause.
Turns out we can and should base this information on the categories
supported by the CT instead, which _may_ loosely resemble who is able
to be source and sink.
> I'm assuming here that the scenario where both devices (local
> and peer) are acting both a2dp roles (which means that audio is
> flowing in both directions) is not realistic and maybe not even
> allowed by the specification.
I had a lot of trouble switching around source and sink on two
machines running BlueZ and pulseaudio, even after using Pali's
excellent A2DP codec work. Tracking down and resolving these issues
took the better part of the weekend and explains why this mail is
late.
There's a proprietary "protocol" called FastStream that allows
bi-directional (SBC) audio streams but I do not have any existing
device to test volume on. The protocol should work between two
BlueZ+PulseAudio devices but that doesn't represent any conclusive
testing.
A more realistic scenario is swapping around source and sink which
works properly after aforementioned fixes (between two devices with
speakers, running BlueZ+PulseAudio). As expected merely the
SEP/transport is recreated, the AVRCP connection is not set up again
meaning the capabilities/features must be static. In other words, we
can't make the distinction between A2DP sink and source profiles in
most of the code here. We'll have to do with exposing the right
category features right from the get-go.
Other than that it appears absolute volume keeps working properly, at
least on the BlueZ side. This requires more intricate testing after I
resolve all issues with my A2DP Absolute Volume implementation in
PulseAudio when used in conjunction with codec- and side-switching.
> Maybe renaming target_supported_events and controller_supported_events
> names for source_supported_events and sink_supported_events would be
> more readable. Because the supported events clearly depend on the a2dp
> role of the local device (and on the avrcp version of the peer).
I don't think we need to separate events out anymore.
> > Indeed, I don't understand this anymore at all. The A2DP role should
> > map directly to the AV Remote (0x110e) and AV Remote Target (0x110c).
> No, where is that written in the specification ? A2DP sink can act
> both AVRCP roles.
>
> > It is my understanding that these define what _direction_ events could
> > possibly go in.
> Yes, "AV Remote (0x110e) and AV Remote Target (0x110c)" define the
> direction of the events.
> AV Remote (0x110e) is the one that sends commands and receives
> notifications and AV Remote Target (0x110c) is the one that receives
> commands and sends notifications.
>
> >For example in 6.13.2 SetAbsoluteVolume we find:
> >
> > It is expected that the audio sink will perform as the TG for this command.
> >
> Exactly, for playback control a2dp sink plays the CT role, but for
> absolute volume it must play TG role. This is exactly proving that the
> a2dp sink role does not force avrcp role. The avrcp role depends on
> the action (playback control or volume control).
>
> > Then, in 6.13.3 Notify Volume Change:
> >
> > This Register Notification event is used by the CT to detect when
> > the volume has been changed locally on the TG, or what the actual
> > volume level is following use of relative volume commands.
> > [...]
> > It is expected for this command that the audio sink fulfills the TG role.
> >
> Another proof for duality of AVRCP roles. For playback control local
> device act the CT role, and for absolute volume the local device act
> TG role.
>
> > That matches what I am trying to explain in all these mails. We
> > cannot assume in this function that we are always the server or TG;
> If we are in any of the control_handlers (for example in
> avrcp_handle_get_capabilities) we for sure are TG (AVRCP target)
> because only CT is allowed to send commands. We just don't know if we
> are a2dp source or sink.
>
> >if
> > we are the audio source, we must be the CT (when Absolute Volume is
> > concerned).
> When we are the audio source we must implement CT role for controlling
> the absolute volume, but that does not mean that in
> avrcp_handle_get_capabilities we play this role.
>
> >This is in accordance with the logical expectation that
> > the sink is in control of the volume.
> Yes, but the sink is not the one that is sending SetAbsoluteVolume
> commands, it is the source that is sending those commands.
>
> >The target (source) can change
> > it by sending SetAbsoluteVolume
> No, target cannot send commands.
>
> >, or ask the controller (sink) to
> > update it about volume changes by means of registering this event.
> No, target cannot send commands so it cannot register events on controller.
>
> > Not the other way around (what would happen if the sink starts sending
> > SetAbsoluteVolume to the source?).
> Yes, sink is not supposed to send SetAbsoluteVolume command, but the
> source act as a CT role in the context of absolute volume. I guess
> that here is the confusion.
Not really, I have always said that the CT is the sender and in case
of SetAbsoluteVolume the source is the sender. I just never made the
link between CT and the RemoteControl service as explained above.
> > What can we do to clarify the difference between CT/TG,
> > target/controller (the profiles), source/sink, client/server?
> > Terminology is going all over the place to the point that I don't know
> > how to express my understanding of the system (nor specs) anymore.
> >
> We should stick strictly to specification terminology.
> 1. target/controller are not profiles, they are roles of the AVRCP
> profile. So the profile is only one which has two roles.
1.1. CT and TG are abbreviations for the controller and target roles
respectively.
> 2. These roles are represented by the services "AV Remote (0x110e)"
> and "AV Remote Target (0x110c)" in the service records. So if one
> device is including "AV Remote (0x110e)" service in one of its
> service records it means it can act the CT role (but this is not
> defining what a2dp role it plays !).
> 3. source/sink are the roles of the A2DP profile. So there is one A2DP
> profile with two roles.
> 4. client/server - don't use that, there is nothing like that in the
> specification.
>
> So bottom line, the way I understand this is that in case when we are
> the a2dp sink:
> 1. if we want to control playback on the a2dp source we must implement
> AVRCP CT role for sending playback commands
> 2, if we want our volume to be controlled by the a2dp source we must
> implement AVRCP TG role for receiving commands.
To clarify, I simply wasn't expecting separate services to denote
whether a peer can send or receive a command. Indeed, it is correct
this way.
> > > > switch (pdu->params[0]) {
> > > > case CAP_COMPANY_ID:
> > > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
>
> > > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
>
> > > > for (i = 0; i < G_N_ELEMENTS(company_ids); i++) {
> > > > @@ -1082,7 +1093,7 @@ static uint8_t avrcp_handle_get_capabilities(struct avrcp *session,
> > > > case CAP_EVENTS_SUPPORTED:
> > > > pdu->params[1] = 0;
> > > > for (i = 1; i <= AVRCP_EVENT_LAST; i++) {
> > > > - if (session->supported_events & (1 << i)) {
> > > > + if (supported_events & (1 << i)) {
> > > > pdu->params[1]++;
> > > > pdu->params[pdu->params[1] + 1] = i;
> > > > }
> > > > @@ -1607,7 +1618,7 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > > > {
> > > > struct avrcp_player *player = target_get_player(session);
> > > > struct btd_device *dev = session->dev;
> > > > - uint16_t len = ntohs(pdu->params_len);
> > > > + uint16_t len = ntohs(pdu->params_len), supported_events;
> > > > uint64_t uid;
> > > > int8_t volume;
> > > >
> > > > @@ -1620,7 +1631,12 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> > > > goto err;
> > > >
> > > > /* Check if event is supported otherwise reject */
> > > > - if (!(session->supported_events & (1 << pdu->params[0])))
> > > > + if (media_transport_is_source(session->dev))
> > > > + supported_events = session->target_supported_events;
> > > > + else
> > > > + supported_events = session->controller_supported_events;
> > > > +
> > > > + if (!(supported_events & (1 << pdu->params[0])))
> > > > goto err;
> > >
> > > Ditto.
> >
> > How do you propose to figure out what "role" we are providing when
> > this function is called? As explained above it depends if we are
> > sink/controller or source/target what events we are expecting to be
> > registered.
> >
> Here we are for sure TG because we are receiving a command. This is
> the A2DP role which is unknown and on which our capabilities depends.
> But I believe that the code here is exactly doing that, it checks if
> we are a2dp source or sink.
>
> > >
> > > > switch (pdu->params[0]) {
> > > > @@ -4129,7 +4145,11 @@ static void target_init(struct avrcp *session)
> > > > media_transport_update_device_volume(session->dev, init_volume);
> > > > }
> > > >
> > > > - session->supported_events |= (1 << AVRCP_EVENT_STATUS_CHANGED) |
> > > > + if (target->version < 0x0103)
> > > > + return;
> > > > +
> > > > + session->target_supported_events |=
> > > > + (1 << AVRCP_EVENT_STATUS_CHANGED) |
> > > > (1 << AVRCP_EVENT_TRACK_CHANGED) |
> > > > (1 << AVRCP_EVENT_TRACK_REACHED_START) |
> > > > (1 << AVRCP_EVENT_TRACK_REACHED_END) |
> > > > @@ -4138,10 +4158,13 @@ static void target_init(struct avrcp *session)
> > > > if (target->version < 0x0104)
> > > > return;
> > > >
> > > > - session->supported_events |=
> > > > + session->target_supported_events |=
> > > > (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
> > > > - (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
> > > > - (1 << AVRCP_EVENT_VOLUME_CHANGED);
> > > > + (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED);
> > > > + // Does not make sense here; the remote is the
> > > > + // rendering device and in control, it'll never
> > > > + // request this notification.
> > > > + // (1 << AVRCP_EVENT_VOLUME_CHANGED);
> > >
> > > Again you are think the roles are based on A2DP roles when they are
> > > not, target supported events should reflect what events we support as
> > > server,
> >
> > I think you are confusing what these variables represent. In the
> > original BlueZ code this value is based on the version of the peer
> > AVRCP_REMOTE_UUID. That to me means we are checking whether we are
> > expecting our peer to register for that particular event, and
> > rejecting it otherwise. I simply extended this to do the same for the
> > remote version of the AVRCP_TARGET_UUID profile as well.
> > What you are suggesting is implicitly encoded as well: if we cannot
> > handle an event registration even though the peer could realistically
> > request it based on its version, it's not in the list.
> >
> supported_events are used only when receiving commands
> (get_capabilietes or register_event) and while receiving those
> commands we play the TG role. So the peer act as CT so it should
> declare AVRCP_REMOTE_UUID(0x110e) service. So only if the peer has
> AVRCP_REMOTE_UUID declared supported_events should be initialised. If
> we are the a2dp source we should declare that we support playback
> control events. If we are a2dp sink we should declare that we support
> absolute volume event.
> So what I believe Luiz is saying is that both supported_events fields
> (target and controller or better name then source and sink) should be
> initialized in target_init.
> But then we have the problem that peer controller wants to register
> absolute volume event before target_init is called.
I guess we have to assign supported_events before setting up the pdu
handler, or at least before sending anything to the peer (like
get_capabilities in controller_init).
> I think that those supported_events should be statically initialised
> separately for a2dp sink and source roles. I cannot understand the
> need for checking the peer side avrcp version, is it against the
> specification if we declare events that are not supported by the peer
> version ? If the peer has avrcp version 1.3 and sends registration
> command of absolute volume event then the peer is not consistent with
> the specification already.
Indeed. I asked the same question at the beginning of this mail but
want to reiterate it as it is core to the problem I tried to fix back
in January. The 1.5 and 1.6.2 versions of the spec mention _nothing_
about the version of the peer CT profile. Instead, absolute volume is
mandatory when the peer CT reports it is a category 2 device.
> > > so we should indicate that we support volume changed even when
> > > acting as a A2DP Source although this seems to be always omitting
> > > volume changed which would be a regression.
> >
> > It does not regress, and was tested with a bluetooth headset to
> > confirm. This is indeed rejecting EVENT_VOLUME_CHANGED, and for a
> > good reason as explained above. If BlueZ is the source, _it_ will
> > register this notification with the headset, not the other way around.
> > Note that get_capabilities_resp/register_notification do not check
> > *_supported_events, and they shouldn't.
> >
> > However, say we flip the roles around and make BlueZ a sink, playing
> > back from an Android phone. Now avrcp_handle_register_notification
> > takes controller_supported_events and will happily comply when the
> > source (the Android phone) registers EVENT_VOLUME_CHANGED.
> >
> > To make this even more explicit insert some log lines in
> > get_capabilities_resp: this clearly shows when connecting headphones
> > as sink it only supports EVENT_VOLUME_CHANGED - when connecting an
> > Android phone as source it reports a bunch of playback/player related
> > events but not EVENT_VOLUME_CHANGED.
> >
> > >
> > > > /* Only check capabilities if controller is not supported */
> > > > if (session->controller == NULL)
> > > > @@ -4180,11 +4203,26 @@ static void controller_init(struct avrcp *session)
> > > > if (controller->version < 0x0103)
> > > > return;
> > > >
> > > > - avrcp_get_capabilities(session);
> > > > + // Players should only run on the remote target; they
> > > > + // should never request notifications about their own
> > > > + // playback status.
> > > > + // session->controller_supported_events |=
> > > > + // (1 << AVRCP_EVENT_STATUS_CHANGED) |
> > > > + // (1 << AVRCP_EVENT_TRACK_CHANGED) |
> > > > + // (1 << AVRCP_EVENT_TRACK_REACHED_START) |
> > > > + // (1 << AVRCP_EVENT_TRACK_REACHED_END) |
> > > > + // (1 << AVRCP_EVENT_SETTINGS_CHANGED);
> > >
> > > The controller/client supported_events should reflect what the remote
> > > target/server supports, so here we should probably not initialize with
> > > anything (or perhaps initialize with the mandatory ones if there are
> > > any events that the spec indicates as mandatory) but read the
> > > supported events from the remote with avrcp_get_capabilities.
> >
> > I think we can initialize this list (as well as
> > target_supported_events) based on what we expect (as explained above),
> > and AND it as soon as get_capabilities_resp tells us what the remote
> > can comply with.
> >
> > > > if (controller->version < 0x0104)
> > > > return;
> > > >
> > > > + session->controller_supported_events |=
> > > > + // (1 << AVRCP_EVENT_ADDRESSED_PLAYER_CHANGED) |
> > > > + // (1 << AVRCP_EVENT_AVAILABLE_PLAYERS_CHANGED) |
> > > > + (1 << AVRCP_EVENT_VOLUME_CHANGED);
> > > > +
> > > > + avrcp_get_capabilities(session);
> > > > +
> > > > if (!(controller->features & AVRCP_FEATURE_BROWSING))
> > > > return;
> > > >
> > > > diff --git a/profiles/audio/transport.c b/profiles/audio/transport.c
> > > > index 8248014ae..f5226776f 100644
> > > > --- a/profiles/audio/transport.c
> > > > +++ b/profiles/audio/transport.c
> > > > @@ -980,3 +980,23 @@ void media_transport_update_device_volume(struct btd_device *dev,
> > > > media_transport_update_volume(transport, volume);
> > > > }
> > > > }
> > > > +
> > > > +gboolean media_transport_is_source(struct btd_device *dev)
> > > > +{
> > > > + GSList *l;
> > > > + const char *uuid;
> > > > +
> > > > + if (dev == NULL)
> > > > + return FALSE;
> > > > +
> > > > + for (l = transports; l; l = l->next) {
> > > > + struct media_transport *transport = l->data;
> > > > + if (transport->device != dev)
> > > > + continue;
> > > > +
> > > > + uuid = media_endpoint_get_uuid(transport->endpoint);
> > > > + return strcasecmp(uuid, A2DP_SOURCE_UUID) == 0;
> > > > + }
> > > > +
> > > > + return FALSE;
> > > > +}
> > > > diff --git a/profiles/audio/transport.h b/profiles/audio/transport.h
> > > > index 51a67ea74..eb1621813 100644
> > > > --- a/profiles/audio/transport.h
> > > > +++ b/profiles/audio/transport.h
> > > > @@ -30,3 +30,4 @@ void transport_get_properties(struct media_transport *transport,
> > > > int8_t media_transport_get_device_volume(struct btd_device *dev);
> > > > void media_transport_update_device_volume(struct btd_device *dev,
> > > > int8_t volume);
> > > > +gboolean media_transport_is_source(struct btd_device *dev);
> > > > --
> > > > 2.29.0
> > > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
> >
> > - Marijn
>
>
>
> --
> mgr inż. Marek Czerski
> +48 696 842 686
- Marijn