Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200118194841.439036-1-marijns95@gmail.com> <CABBYNZ+LW-Lp3q9jOjVwFktuLsKajTrmOpUYmQ5SqCUPrGQ7BA@mail.gmail.com>
In-Reply-To: <CABBYNZ+LW-Lp3q9jOjVwFktuLsKajTrmOpUYmQ5SqCUPrGQ7BA@mail.gmail.com>
From:   Marijn <marijns95@gmail.com>
Date:   Wed, 22 Jan 2020 01:16:32 +0100
Message-ID: <CANX-K3vEopmiSATkZyDS38CJ0_uTWd0ArKdyJi1UYQRCN2pELQ@mail.gmail.com>
Subject: Re: [BlueZ PATCH] audio: avrcp: Ignore peer RT supported_events when
 being the RT.
To:     Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        Luiz Augusto Von Dentz <luiz.von.dentz@intel.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

Hi Luiz,

On Tue, 21 Jan 2020 at 23:48, Luiz Augusto von Dentz
<luiz.dentz@gmail.com> wrote:
>
> Hi Marijn,
>
> On Sat, Jan 18, 2020 at 11:52 AM Marijn Suijten <marijns95@gmail.com> wrote:
> >
> > From: Marijn Suijten <marijns95@gmail.com>
> >
> > Remove the check of a received event against supported_events in
> > avrcp_handle_register_notification, which is only called when BlueZ is
> > the RT even though supported_events is only valid when BlueZ is the TG.
> >
> > supported_events is assigned in target_init with events that the
> > corresponding RT on the other side of the Bluetooth connection supports,
> > to ensure the local TG will never report anything unexpected in
> > avrcp_handle_get_capabilities. This value is specific to what the target
> > should support to be compatible with the peer RT, but a locally running
> > RT has nothing to do with the external device being the RT.
> >
> > This addresses the case where Absolute Volume notification registrations
> > are rejected when audio is played from an Android device to a computer
> > running BlueZ. The Android BT stack report an RT of version 1.3 [1]
> > which does not include Absolute Volume support. The RT on the Android
> > device is not involved in such a playback session, instead the computer
> > is the RT and the Android device the TG.
> >
> > This has been tested by disabling registration of the RT service in
> > Android, to make the device a "pure" media player that cannot receive
> > audio: target_init does not get called and supported_events stays 0
> > which would have caused any notification registration to be rejected in
> > the above case.
>
> I assume you have a typo on RT instead of CT, is that right? From
> qualification point of view anything initiated by a device is
> considered a CT role, much like GATT server and client roles, so we
> may have instances where both CT and TG are supported simultaneously.

I have indeed missed this distinction entirely. Looking at the AVRCP
spec (1.6) section 2.2.1 "Roles" the CT/TG naming indeed denotes
initiator and receiver, respectively. I blindly assumed these matched
the roles of "target" and "remote"/"controller" following the naming
of the profiles. Knowing this makes reading section 6.18 much more
clear; the sink is assumed to be the TG only in the specific case of
the SetAbsoluteVolume call, for example. I will update the commit to
mention "target" and "controller" instead (like the rest of the code),
hopefully finding and clearing up the typo that way.

>
> > [1]: https://android.googlesource.com/platform/system/bt/+/android-10.0.0_r25/bta/av/bta_av_main.cc#712
> > ---
> > Hi,
> >
> > I have a separate patch lying around that - instead of removing
> > supported_events - splits it up in two variables; one for the target and
> > another for the controller. Let me know if this extra safety is desired.
> >
> > According to the AVRCP 1.3 specification GetCapabilities is mandatory,
> > which I have included in that patch. However the documentation also
> > mentions that this function is only supposed to be called by the CT
> > meaning that the call in target_init (introduced in 27efc30f0) is not
> > valid. What is your view on this?
> > Unfortunately even the small pair of in-ears I have lying around report
> > AVRCP TG functionality while they are not nearly capable of being a
> > target/media-source, so I have not been able to confirm how a pure RT
> > device would respond in such case.
>
> As I mentioned above the qualification tests requires both TG and CT
> for things like absolute volume to work as notifications for volume
> changes originate from the device rendering the audio/sink not the
> source, so the typical association of sink/CT, source/TG is no longer
> true and before you ask, yes we have some code and comments leading to
> that assumption which we should probably fix at some point so I guess
> having the supported events in 2 is probably a good idea, though
> notice that it should probably be local and remote events since event
> afaik are always originated from the TG role.

If there is any code it must be very minor, as I have based the
sink/CT, source/TG association solely on the AVRCP documentation. I do
see it in the creation of of sdp records though, is that what you're
hinting at?
According to the documentation it is likely the CT requests a
notification, and the TG completes it in due time. But because both
the controller and the target can fulfill the CT and TG role (keeping
in mind predefined directionality in for example SetAbsoluteVolume), I
don't think that distinction should make it into my patch. Instead,
the separation will be solely based on what the local/peer controller
and target support. The issue still holds that BlueZ in controller
role incorrectly rejects notification registrations, because
supported_events is merely based on the peer controller version.
Splitting supported events seems the way forward: I'll improve the
patch and resubmit it, then we can decide. Likewise, should
registered_events be separated across the target and controller role
too? Or is it unexpected to have a target _and_ controller running on
both devices, simultaneously (this would imply two transports, in both
directions, playing back media)?

Locally supported events seem hard to quantify as BlueZ can send out
events for anything that's physically in the code at any point
(supposedly anything available according to AVRCP_{CT,TG}_VERSION),
unless we want to prevent the CT from requesting notifications it
couldn't realistically support given its profile version.

>
> >
> > - Marijn Suijten
> >
> >  profiles/audio/avrcp.c | 4 ----
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/profiles/audio/avrcp.c b/profiles/audio/avrcp.c
> > index 7fb8af608..820494d2a 100644
> > --- a/profiles/audio/avrcp.c
> > +++ b/profiles/audio/avrcp.c
> > @@ -1529,10 +1529,6 @@ static uint8_t avrcp_handle_register_notification(struct avrcp *session,
> >         if (len != 5)
> >                 goto err;
> >
> > -       /* Check if event is supported otherwise reject */
> > -       if (!(session->supported_events & (1 << pdu->params[0])))
> > -               goto err;
>
> When receiving a request our role is TG so I don't see why we would
> skip this check, a better way to fix this would be to add the
> separated supported events like discussed above so we have the roles
> operating independent as they should be.

I will update this to take into account the target/controller role at
the time the call is made. Is there any way to figure out what role
the TG is fulfilling? set_volume in transport.c for example bases that
on whether the transport has source_watch set, meaning the device is
the sink playing back audio from a peer device which is the source.

>
> >         switch (pdu->params[0]) {
> >         case AVRCP_EVENT_STATUS_CHANGED:
> >                 len = 2;
> > --
> > 2.25.0
> >
>
>
> --
> Luiz Augusto von Dentz

- Marijn