Hi Marijn, On Sat, Jan 18, 2020 at 11:52 AM Marijn Suijten <marijns95@xxxxxxxxx> wrote: > > From: Marijn Suijten <marijns95@xxxxxxxxx> > > 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. > [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. > > - 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. > switch (pdu->params[0]) { > case AVRCP_EVENT_STATUS_CHANGED: > len = 2; > -- > 2.25.0 > -- Luiz Augusto von Dentz
