Closed Bug 1095488 Opened 10 years ago Closed 10 years ago

[Bluetooth] Implement AVRCP support in daemon backend

Categories

(Firefox OS Graveyard :: Bluetooth, defect)

Product:
Firefox OS Graveyard
Component:
Bluetooth
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(feature-b2g:2.2+)

Status:
RESOLVED FIXED
Milestone:
2.2 S3 (9jan)
Project Flags:
feature-b2g 2.2+

People

(Reporter: tzimmermann, Assigned: tzimmermann)

References

Details

Attachments

(4 files, 8 obsolete files)

[03] Bug 1095488: Added Bluetooth AVRCP interface
9.60 KB, patch
shawnjohnjr
: review+
ben.tian
: feedback+
Details | Diff | Splinter Review
[01] Bug 1095488: Add Bluetooth AVRCP helpers (v2)
24.04 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[02] Bug 1095488: Add Bluetooth AVRCP module (v2)
36.40 KB, patch
shawnjohnjr
: review+
Details | Diff | Splinter Review
[04] Bug 1095488: Added AVRCP support for Bluetooth daemon backend (v2)
7.81 KB, patch
tzimmermann
: review+
Details | Diff | Splinter Review
Assignee: nobody → tzimmermann
Status: NEW → ASSIGNED
feature-b2g: --- → 2.2+
Thank you, will review it today.
Hi Shawn (In reply to Shawn Huang [:shawnjohnjr] from comment #5) > Thank you, will review it today. You probably confused bug numbers here, but these patches are not yet ready. Please review the patches in bug 1091588 instead. :)
Oh, yes, I found myself replied the wrong bug number due to too many tabs opened.
Attachment #8522907 - Attachment is obsolete: true
Attachment #8534974 - Flags: review?(shuang)
Attachment #8534974 - Flags: feedback?(btian)
Attachment #8522908 - Attachment is obsolete: true
Attachment #8534975 - Flags: review?(shuang)
Attachment #8534975 - Flags: feedback?(btian)
Attachment #8534974 - Attachment description: [01] Bug 1095487: Add Bluetooth A2DP helpers → [01] Bug 1095487: Add Bluetooth AVRCP helpers
Attachment #8522909 - Attachment is obsolete: true
Attachment #8534976 - Flags: review?(shuang)
Attachment #8534976 - Flags: feedback?(btian)
Attachment #8522910 - Attachment is obsolete: true
Attachment #8534977 - Flags: review?(shuang)
Attachment #8534977 - Flags: feedback?(btian)
Final feature patch set. :) Once we have this in Gecko, we can start switching over to the daemon. I included a lengthy comment in [04] that explains the relationship between |BluetoothDaemonProtocol| and the modules.
Comment on attachment 8534974 [details] [diff] [review] [01] Bug 1095487: Add Bluetooth AVRCP helpers Review of attachment 8534974 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #8534974 - Flags: review?(shuang) → review+
Comment on attachment 8534974 [details] [diff] [review] [01] Bug 1095487: Add Bluetooth AVRCP helpers I did not see Remote feature cnoversion function in Patch 1. Is there something I missed?
Attachment #8534974 - Flags: review+
Comment on attachment 8534975 [details] [diff] [review] [02] Bug 1095488: Add Bluetooth AVRCP module Review of attachment 8534975 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.cpp @@ +55,5 @@ > + > +nsresult > +BluetoothDaemonAvrcpModule::GetPlayStatusRspCmd( > + ControlPlayStatus aPlayStatus, uint32_t aSongLen, uint32_t aSongPos, > + BluetoothAvrcpResultHandler* aRes ) nit:empty space before ')' @@ +60,5 @@ > +{ > + MOZ_ASSERT(NS_IsMainThread()); > + > + nsAutoPtr<BluetoothDaemonPDU> pdu( > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_GET_PLAY_STATUS_RSP, 9)); nit:Add comments for constant 9. @@ +256,5 @@ > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_REGISTER_NOTIFICATION_RSP, > + 1 + // Event > + 1 + // Type > + 1 + // Data length > + 256)); // Maximum data length Do we need to allocate update to 256 here? Or it's just optimization. @@ +494,5 @@ > + if (NS_FAILED(rv)) { > + return rv; > + } > + > + /* Read feature */ I did not see Remote feature cnoversion function in Patch 1. Is there something I missed? ::: dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.h @@ +56,5 @@ > + OPCODE_GET_PLAYER_APP_VALUES_TEXT_NTF = 0x86, > + OPCODE_SET_PLAYER_APP_VALUE_NTF = 0x87, > + OPCODE_GET_ELEMENT_ATTR_NTF = 0x88, > + OPCODE_REGISTER_NOTIFICATION_NTF = 0x89, > + OPCODE_VOLUME_CHANGE_NTF = 0x8a, Both OPCODE_VOLUME_CHANGE_NTF, OPCODE_PASSTHROUGH_CMD_NTF does not support in ANDROID_VERSION = 18 (Jelly Bean/4.3). But I guess we don't care here.
Comment on attachment 8534977 [details] [diff] [review] [04] Bug 1095488: Added AVRCP support for Bluetooth daemon backend Review of attachment 8534977 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #8534977 - Flags: review?(shuang) → review+
Comment on attachment 8534974 [details] [diff] [review] [01] Bug 1095487: Add Bluetooth AVRCP helpers Review of attachment 8534974 [details] [diff] [review]: ----------------------------------------------------------------- f=me with comment 14 |RemoteFeature| conversion added.
Attachment #8534974 - Flags: feedback?(btian) → feedback+
I tested and try to fetch AVRCP 1.3 metadata but it seems that PDU overflow. 12-18 15:39:27.888 D/bt-avp ( 222): avrc_msg_cback handle:1, ctype:1, offset:11, len: 19 12-18 15:39:27.888 E/bt-avp ( 222): opcode 0 12-18 15:39:27.888 D/bt-avp ( 222): pkt_type 0 12-18 15:39:27.888 I/bt-btif ( 222): bta_av_rc_msg_cback handle: 1 opcode=0x0 12-18 15:39:27.888 I/bt-btif ( 222): BTA got event 0x1207 12-18 15:39:27.888 I/bt-btif ( 222): AV event=0x1207(AVRC_MSG) state=1(OPEN) 12-18 15:39:27.888 I/bt-btif ( 222): next state=1 12-18 15:39:27.888 D/bt-btif ( 222): bta_av_rc_msg opcode: 0 12-18 15:39:27.888 D/bt-btif ( 222): btif_av_state_opened_handler event:BTA_AV_META_MSG_EVT flags 0 12-18 15:39:27.888 D/bt-avp ( 222): avrc_pars_vendor_cmd() pdu:0x20 12-18 15:39:27.888 D/bt-avp ( 222): AVRC_ParsCommand() return status:0x4 12-18 15:39:27.888 E/ ( 222): PDU overflow <----------overflow
Breakpoint 1, read_pdu_at_va (ap=..., fmt=<optimized out>, offset=3, pdu=0xb730afc0) at system/bluetoothd/src/bt-proto.c:141 141 ALOGE("PDU overflow"); (gdb) bt #0 read_pdu_at_va (ap=..., fmt=<optimized out>, offset=3, pdu=0xb730afc0) at system/bluetoothd/src/bt-proto.c:141 #1 read_pdu_at (pdu=pdu@entry=0xb730afc0, offset=<optimized out>, fmt=0xb6f42799 "m") at system/bluetoothd/src/bt-proto.c:158 #2 0xb6f409d8 in read_btrc_element_attr_val_t (pdu=pdu@entry=0xb730afc0, off=<optimized out>, attr=attr@entry=0xb73229a8) at system/bluetoothd/src/bt-proto.c:408 #3 0xb6f40a00 in read_btrc_element_attr_val_t_array (pdu=pdu@entry=0xb730afc0, off=off@entry=1, attr=attr@entry=0xb73229a8, num_attrs=7) at system/bluetoothd/src/bt-proto.c:422 #4 0xb6f41374 in opcode_get_element_attr_rsp (cmd=0xb730afc0) at system/bluetoothd/src/bt-rc-io.c:609 #5 0xb6f40684 in handle_pdu (handler=<optimized out>, cmd=<optimized out>, value=<optimized out>, field=0xb6f42927 "opcode") at system/bluetoothd/src/bt-proto.c:50 #6 handle_pdu_by_opcode (cmd=<optimized out>, handler=<optimized out>) at system/bluetoothd/src/bt-proto.c:64 #7 0xb6f40658 in handle_pdu (handler=<optimized out>, cmd=0xb730afc0, value=<optimized out>, field=0xb6f4291f "service") at system/bluetoothd/src/bt-proto.c:50 #8 handle_pdu_by_service (cmd=cmd@entry=0xb730afc0, handler=<optimized out>) at system/bluetoothd/src/bt-proto.c:57 #9 0xb6f3f248 in handle_pdu (cmd=0xb730afc0) at system/bluetoothd/src/bt-io.c:245 (gdb) p pdu->len $1 = 66 (gdb) p offset+len $2 = 90
Comment on attachment 8534976 [details] [diff] [review] [03] Bug 1095488: Added Bluetooth AVRCP interface Review of attachment 8534976 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #8534976 - Flags: review?(shuang) → review+
(In reply to Shawn Huang [:shawnjohnjr] from comment #19) > Breakpoint 1, read_pdu_at_va (ap=..., fmt=<optimized out>, offset=3, > pdu=0xb730afc0) at system/bluetoothd/src/bt-proto.c:141 > 141 ALOGE("PDU overflow"); > 12-18 15:39:27.888 D/bt-avp ( 222): avrc_pars_vendor_cmd() pdu:0x20 > 12-18 15:39:27.888 D/bt-avp ( 222): AVRC_ParsCommand() return status:0x4 > 12-18 15:39:27.888 E/ ( 222): PDU overflow <----------overflow I got some problems with my AVRCP 1.3 test machine. So last week, I did not have much progress, expect this week i can debug further and find out why PDU overflow.
Comment on attachment 8534977 [details] [diff] [review] [04] Bug 1095488: Added AVRCP support for Bluetooth daemon backend Review of attachment 8534977 [details] [diff] [review]: ----------------------------------------------------------------- f=me with comment addressed. ::: dom/bluetooth/bluedroid/BluetoothDaemonInterface.cpp @@ +1387,5 @@ > +// connections of type |BluetoothDaemonConnection| invoke this method > +// to forward received PDUs for processing by higher layers. The > +// implementation of |Handle| checks the service id of the PDU and > +// forwards it to the correct module class using the module's method > +// |HandleSvc|. Further PDU processing it module-dependent. typo: I think you mean "is module-dependent."
Attachment #8534977 - Flags: feedback?(btian) → feedback+
Comment on attachment 8534976 [details] [diff] [review] [03] Bug 1095488: Added Bluetooth AVRCP interface Review of attachment 8534976 [details] [diff] [review]: ----------------------------------------------------------------- LGTM
Attachment #8534976 - Flags: feedback?(btian) → feedback+
Comment on attachment 8534975 [details] [diff] [review] [02] Bug 1095488: Add Bluetooth AVRCP module Review of attachment 8534975 [details] [diff] [review]: ----------------------------------------------------------------- f=me with comment 15 addressed.
Attachment #8534975 - Flags: feedback?(btian) → feedback+
Target Milestone: --- → 2.2 S3 (9jan)
Attached patch fix_avrcp.patch (obsolete) — — Splinter Review
Hi Shawn, I think the current encoder for message 0x07 doesn't pack the string length. Could you (or anyone else) apply this quick fix on top of the other patches and test again? I'll upload a cleaned up patch set soon.
Attachment #8544057 - Flags: feedback?(shuang)
(In reply to Shawn Huang [:shawnjohnjr](PTO: 12/28 ~ 1/6] from comment #14) > Comment on attachment 8534974 [details] [diff] [review] > [01] Bug 1095487: Add Bluetooth AVRCP helpers > > I did not see Remote feature cnoversion function in Patch 1. Is there > something I missed? That did a direct conversion to 'unsigned long'. I added the conversion to BluetoothAvrcpRemoteFeature.
> > @@ +256,5 @@ > > + new BluetoothDaemonPDU(SERVICE_ID, OPCODE_REGISTER_NOTIFICATION_RSP, > > + 1 + // Event > > + 1 + // Type > > + 1 + // Data length > > + 256)); // Maximum data length > > Do we need to allocate update to 256 here? Or it's just optimization. 256 Byte is the maximum data length. To get the exact length, we'd have to decode the content of |aParam| which depends on |aEvent|. It seems not worth the effort, just to save a few bytes in the allocation. OK? > > ::: dom/bluetooth/bluedroid/BluetoothDaemonAvrcpInterface.h > @@ +56,5 @@ > > + OPCODE_GET_PLAYER_APP_VALUES_TEXT_NTF = 0x86, > > + OPCODE_SET_PLAYER_APP_VALUE_NTF = 0x87, > > + OPCODE_GET_ELEMENT_ATTR_NTF = 0x88, > > + OPCODE_REGISTER_NOTIFICATION_NTF = 0x89, > > + OPCODE_VOLUME_CHANGE_NTF = 0x8a, > > Both OPCODE_VOLUME_CHANGE_NTF, OPCODE_PASSTHROUGH_CMD_NTF does not support > in ANDROID_VERSION = 18 (Jelly Bean/4.3). But I guess we don't care here. Oh, good catch! I refered to BlueZ 5.14 for these old versions and the daemon handles this correctly. I guess I must have missed it here.
Changes since v1: - added unpacking conversion for BluetoothAvrcpRemoteFeature - fixed packing of command 0x07
Attachment #8534974 - Attachment is obsolete: true
Attachment #8544625 - Flags: review?(shuang)
Changes since v1: - removed unsupported notifications from JB platform - fixed unpack conversion of BluetoothAvrcpRemoteFeature - fixed typos
Attachment #8534975 - Attachment is obsolete: true
Attachment #8544626 - Flags: review?(shuang)
Changes since v1: - fixed typos
Attachment #8534977 - Attachment is obsolete: true
Attachment #8544627 - Flags: review+
Attachment #8544057 - Attachment is obsolete: true
Attachment #8544057 - Flags: feedback?(shuang)
Setting the feedback flag again to get some testing.
Flags: needinfo?(shuang)
Great! I tried the new patch. The overflow problem is gone, but I saw error log shows: 01-07 15:19:04.916 D/bt-btif ( 223): btif_av_state_opened_handler event:BTA_AV_META_MSG_EVT flags 0 01-07 15:19:04.916 D/bt-avp ( 223): avrc_pars_vendor_cmd() pdu:0x20 01-07 15:19:04.916 D/bt-avp ( 223): AVRC_ParsCommand() return status:0x4 01-07 15:19:04.916 E/BTIF_RC ( 223): ### ASSERT : external/bluetooth/bluedroid/main/../btif/src/btif_rc.c line 1312 Callback is NULL (0) ### It seems like get_element_attr_cb callback is null. HAL_CBACK(bt_rc_callbacks, get_element_attr_cb, num_attr, element_attrs);
(In reply to Shawn Huang [:shawnjohnjr] from comment #32) > Great! I tried the new patch. The overflow problem is gone, but I saw error > log shows: > 01-07 15:19:04.916 D/bt-btif ( 223): btif_av_state_opened_handler > event:BTA_AV_META_MSG_EVT flags 0 > 01-07 15:19:04.916 D/bt-avp ( 223): avrc_pars_vendor_cmd() pdu:0x20 > 01-07 15:19:04.916 D/bt-avp ( 223): AVRC_ParsCommand() return status:0x4 > 01-07 15:19:04.916 E/BTIF_RC ( 223): ### ASSERT : > external/bluetooth/bluedroid/main/../btif/src/btif_rc.c line 1312 Callback > is NULL (0) ### > > It seems like get_element_attr_cb callback is null. > HAL_CBACK(bt_rc_callbacks, get_element_attr_cb, num_attr, > element_attrs); Sorry. Please ignore this comment. I found my local patch does not apply correctly. Sorry for misleading.
Flags: needinfo?(shuang)
(In reply to Shawn Huang [:shawnjohnjr] from comment #33) > (In reply to Shawn Huang [:shawnjohnjr] from comment #32) > > Great! I tried the new patch. The overflow problem is gone, but I saw error > > log shows: > > 01-07 15:19:04.916 D/bt-btif ( 223): btif_av_state_opened_handler > > event:BTA_AV_META_MSG_EVT flags 0 > > 01-07 15:19:04.916 D/bt-avp ( 223): avrc_pars_vendor_cmd() pdu:0x20 > > 01-07 15:19:04.916 D/bt-avp ( 223): AVRC_ParsCommand() return status:0x4 > > 01-07 15:19:04.916 E/BTIF_RC ( 223): ### ASSERT : > > external/bluetooth/bluedroid/main/../btif/src/btif_rc.c line 1312 Callback > > is NULL (0) ### > > > > It seems like get_element_attr_cb callback is null. > > HAL_CBACK(bt_rc_callbacks, get_element_attr_cb, num_attr, > > element_attrs); > > Sorry. Please ignore this comment. I found my local patch does not apply > correctly. Sorry for misleading. OK, thanks for reporting back. The patches applied cleanly yesterday BTW. Please let me know if I need to rebase them.
Comment on attachment 8544625 [details] [diff] [review] [01] Bug 1095488: Add Bluetooth AVRCP helpers (v2) Review of attachment 8544625 [details] [diff] [review]: ----------------------------------------------------------------- It looks good to me. r=me. Thanks.
Attachment #8544625 - Flags: review?(shuang) → review+
Additional information: I tested meta data functionality with AVRCP emulator, it seems both Artist/Album name/Track No# can be correctly displayed.
Blocks: 1118693
I'm checking why no response from get play status command. Maybe something wrong with my emulator, i will update the status later and if needed, I will open an another bug anyway for tracking getting play-status problem.
(In reply to Shawn Huang [:shawnjohnjr] from comment #38) > I'm checking why no response from get play status command. Maybe something > wrong with my emulator, i will update the status later and if needed, I will > open an another bug anyway for tracking getting play-status problem. I think this actually related to my testing environment. Get-Play-Status can be correctly replied. Thanks.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: