1142417 - [Bluetooth][PTS][2.2] AVRCP- TC_TG_RCR_BV_02_C failed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Bluetooth, defect)

Description

[Shing Lyu [:shinglyu]]

The test runs forever until timed out

##STR
Test case : TC_TG_RCR_BV_02_C started
	- MTC: SDP Service record was successfully registered.
	- A2DP SNK SDP Service record successfully registered
	- A2DP SNKsuccessfully registered
	- MTC: IUT successfully responded to AVDTP CONNECT.
	- MTC: IUT successfully accepted AVCTP channel connection.
	- MTC: Guard timer timed out.
	- Guard timer timed out
	- AVCTP: Exiting AVCTP1 PTC
	- AVDTP: Exiting AVDTP1 PTC
	- CM_PTC_EXIT
	- MMI CM_EXIT
	- MTC: Test case ended
Final Verdict:INCONC
TC_TG_RCR_BV_02_C finished


## Version
* V18D
* 2014/03/12's pvt build for 2.2
* adb shell setprop ro.moz.bluetooth.backend bluetoothd

Comment 1

[Shing Lyu [:shinglyu]]

Attachment: TC_TG_RCR_BV_02_C_2015_03_11_17_30_47.zip

Comment 2

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

This looks like you have not yet paired before. Due to the test case needs to establish connection from PTS machine (incoming connection). So I would suggest you can run one HFP AG-AG_ATA_BV_02_I to make sure you paired with PTS machine, then you can cancel AG-AG_ATA_BV_02_I test case and start to test AVRCP test cases.

PTS cannot handle this situation if you haven't pair with phone (if you don't pair the incoming connection will be blocked by the phone).

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

This test case tested AVRCP can correctly handle fragmentation.

Even applied patch from bug 1142408, 1144089(no music title), this test case (fragmented Get Element) seems still fail to pass.

03-16 17:27:56.800 I/bt-btif (  905): + handle_rc_metamsg_cmd
03-16 17:27:56.800 D/bt-btif (  905): Received vendor command.code,PDU and label: 1, 32,10
03-16 17:27:56.800 I/bt-btif (  905): handle_rc_metamsg_cmd: Passing received metamsg command to app. pdu: AVRC_PDU_GET_ELEMENT_ATTR
03-16 17:27:56.800 I/bt-btif (  905): btif_rc_upstreams_evt pdu: AVRC_PDU_GET_ELEMENT_ATTR handle: 0x0 ctype:1 label:a
03-16 17:27:56.800 I/bt-btif (  905): HAL bt_rc_callbacks->get_element_attr_cb

but i don't see any get element respond command sent, need to digging into further.

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Shawn Huang [:shawnjohnjr] from comment #3)
> This test case tested AVRCP can correctly handle fragmentation.
> 
> Even applied patch from bug 1142408, 1144089(no music title), this test case
> (fragmented Get Element) seems still fail to pass.
> 
> 03-16 17:27:56.800 I/bt-btif (  905): + handle_rc_metamsg_cmd
> 03-16 17:27:56.800 D/bt-btif (  905): Received vendor command.code,PDU and
> label: 1, 32,10
> 03-16 17:27:56.800 I/bt-btif (  905): handle_rc_metamsg_cmd: Passing
> received metamsg command to app. pdu: AVRC_PDU_GET_ELEMENT_ATTR
> 03-16 17:27:56.800 I/bt-btif (  905): btif_rc_upstreams_evt pdu:
> AVRC_PDU_GET_ELEMENT_ATTR handle: 0x0 ctype:1 label:a
> 03-16 17:27:56.800 I/bt-btif (  905): HAL
> bt_rc_callbacks->get_element_attr_cb
> 
> but i don't see any get element respond command sent, need to digging into
> further.

Test case needs to verify meta data over 512 bytes, in order to verify DUT can handle AVRCP fragmentation. Bluetooth stack should be able to handle it, however Gecko never send PDU to bluetoothd.

It looks like PackPDU failed when executing GetElementAttrRspCmd. 
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.cpp#220
https://dxr.mozilla.org/mozilla-central/source/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h#506

Comment 5

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

nsresult
Convert(uint32_t aIn, uint8_t& aOut)
{
  if (NS_WARN_IF(aIn < std::numeric_limits<uint8_t>::min()) ||
      NS_WARN_IF(aIn > std::numeric_limits<uint8_t>::max())) {
    aOut = 0; // silences compiler warning
    return NS_ERROR_ILLEGAL_VALUE;
....

#0  0xb584e942 in mozilla::dom::bluetooth::Convert (aIn=260, aOut=@0xbee7e70f: 0 '\000') at /code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp:573
#1  0xb58501ec in mozilla::dom::bluetooth::PackPDU<unsigned int, unsigned char> (aIn=..., aPDU=...) at /code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h:506
#2  0xb585025c in mozilla::dom::bluetooth::PackPDU (aIn=..., aPDU=...) at /code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp:1245
#3  0xb584acd4 in PackPDU<mozilla::dom::bluetooth::BluetoothAvrcpElementAttribute> (aPDU=..., aIn=...) at /code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h:538
#4  PackPDU<unsigned char, mozilla::dom::bluetooth::PackArray<mozilla::dom::bluetooth::BluetoothAvrcpElementAttribute> > (aPDU=..., aIn2=..., aIn1=<optimized out>)
    at /code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.h:585
#5  mozilla::dom::bluetooth::BluetoothDaemonAvrcpModule::GetElementAttrRspCmd (this=0xafd75058, aNumAttr=<optimized out>, aAttr=0xae336208, aRes=aRes@entry=0x0)
    at /code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.cpp:219
#6  0xb584bca2 in mozilla::dom::bluetooth::BluetoothDaemonAvrcpInterface::GetElementAttrRsp (this=0xafd49f70, aNumAttr=<optimized out>, aAttr=<optimized out>, aRes=0x0)
    at /code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.cpp:1043
#7  0xb5848770 in mozilla::dom::bluetooth::BluetoothA2dpManager::GetElementAttrNotification (this=<optimized out>, aNumAttrs=<optimized out>, aAttrs=0xac840020)
    at /code/b2g37_v2_2/mozilla-b2g37_v2_2/dom/bluetooth/bluedroid/BluetoothA2dpManager.cpp:1104

Comment 6

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/file/7a9f2a248e57/dom/bluetooth/bluedroid/BluetoothDaemonHelpers.cpp#l1122
Oh, the test sample that the tester uses the song title length is over 255 and we do conversion to convert PRUint32 to uint8_t. This PTS test case is trying to test AVRCP media item with a least 512 bytes worth of metadata.

Based on AVRCP spec Table 6.27: GetElementAttributes response, AttributeValueLength1 (Length of the value of the
attribute), allowed values are 0-65535. However, based on [1], that ipc protocol only allows 1 octet.

[1] https://git.kernel.org/cgit/bluetooth/bluez.git/tree/android/hal-ipc-api.txt?id=5.29#n1295

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Get Element Attributes Response format (avrcp spec)

Comment 8

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hello Thomas
Because the AVRCP test suite considers this test case as mandatory. Can we change bluez protocol (probably we need to feedback to bluez team)? Please reference Comment 6, Comment 7.

Comment 9

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Hi Shawn,

I checked the current interfaces of Android and it occurs to me that there's no way to pass more then 255 bytes of text to the functions. That's the constant BTRC_MAX_ATTR_STR_LEN, which is defines in the AVRCP header file. [1]

It this a new test? Or do you know why the old implementation passed it?

[1] https://android.googlesource.com/platform/hardware/libhardware/+/android-5.1.0_r1/include/hardware/bt_rc.h

Comment 10

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> Hi Shawn,
> 
> I checked the current interfaces of Android and it occurs to me that there's
> no way to pass more then 255 bytes of text to the functions. That's the
> constant BTRC_MAX_ATTR_STR_LEN, which is defines in the AVRCP header file.
> [1]
> 
> It this a new test? Or do you know why the old implementation passed it?
> 
> [1]
> https://android.googlesource.com/platform/hardware/libhardware/+/android-5.1.
> 0_r1/include/hardware/bt_rc.h
It's not new test case, and old implementation can pass it.
What Android only copies the maximum length string.
http://androidxref.com/5.1.0_r1/xref/packages/apps/Bluetooth/jni/com_android_bluetooth_avrcp.cpp#294

I also switch backend to in-gecko HAL interface and test case can pass.

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Results-Get Element Attr Response (in-geckoHALInterface)

(In reply to Shawn Huang [:shawnjohnjr] from comment #10)
> (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> 
> I also switch backend to in-gecko HAL interface and test case can pass.
I checked the dump, max length will be 254. So maybe we can trip the characters.

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> It this a new test? Or do you know why the old implementation passed it?
The old implementation can pass it because bluedroid internally trims length to 254 even the parameters are over 255, and bluetoothd backend code returns NS_ERROR_ILLEGAL_VALUE directly.

Comment 13

[Thomas Zimmermann [:tzimmermann] [:tdz]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #11)
> Created attachment 8582901 [details]
> Results-Get Element Attr Response (in-geckoHALInterface)
> 
> (In reply to Shawn Huang [:shawnjohnjr] from comment #10)
> > (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> > 
> > I also switch backend to in-gecko HAL interface and test case can pass.
> I checked the dump, max length will be 254. So maybe we can trip the
> characters.

That sounds good to me. I'd prefer to crop the trailing characters in Gecko and issue a warning into the log. When Android starts supporting longer strings, we can still update the protocol. Would that be OK for the test?

Comment 14

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #13)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #11)
> > Created attachment 8582901 [details]
> > Results-Get Element Attr Response (in-geckoHALInterface)
> > 
> > (In reply to Shawn Huang [:shawnjohnjr] from comment #10)
> > > (In reply to Thomas Zimmermann [:tzimmermann] [:tdz] from comment #9)
> > > 
> > > I also switch backend to in-gecko HAL interface and test case can pass.
> > I checked the dump, max length will be 254. So maybe we can trip the
> > characters.
> 
> That sounds good to me. I'd prefer to crop the trailing characters in Gecko
> and issue a warning into the log. When Android starts supporting longer
> strings, we can still update the protocol. Would that be OK for the test?

Yes. I think that should be ok.

Comment 15

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1142417 - Truncate AVRCP meta data strings followed by BTRC_MAX_ATTR_STR_LEN

Comment 16

[Thomas Zimmermann [:tzimmermann] [:tdz]]

Comment on attachment 8584482 [details] [diff] [review]
Bug 1142417 - Truncate AVRCP meta data strings followed by BTRC_MAX_ATTR_STR_LEN

Review of attachment 8584482 [details] [diff] [review]:
-----------------------------------------------------------------

Makes sense. Maybe we should also put a warning into the log when truncating the string.

Comment 17

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1142417 - Truncate AVRCP meta data strings followed by BTRC_MAX_ATTR_STR_LEN, r=tzimmermann

Comment 18

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Comment on attachment 8584506 [details] [diff] [review]
Bug 1142417 - Truncate AVRCP meta data strings followed by BTRC_MAX_ATTR_STR_LEN, r=tzimmermann

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): daemon feature/bluedroid limitation
User impact if declined: Can not send correct length of data to pass certification
Testing completed: Pass PTS test case
Risk to taking this patch (and alternatives if risky):AVRCP Control role cannot receive attribute string length over 254 due to bluedroid limitation. I expected normally media title/artist/album name won't exceed up to 254 
String or UUID changes made by this patch: None

Comment 19

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

VerifyMe:
I have tested both TC_TG_RCR_BV_02_V and TC_TG_RCR_BV_04_V pass.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/174750113959

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/174750113959

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/6a0fe86e1c3b