Closed Bug 897882 Opened 7 years ago Closed 6 years ago

[Music] Support bluetooth AVRCP 1.0 + 1.3


(Firefox OS Graveyard :: Gaia::Music, defect)

Gonk (Firefox OS)
Not set



blocking-b2g koi+


(Reporter: dkuo, Assigned: dkuo)



(Whiteboard: [u=devices c=BT p=0] )


(2 files)

This bug is filed for the integration of bluetooth AVRCP 1.0 and Music app.

Since the proposed MediaRemoteControl api is not ready yet, and bluetooth team and QA need AVRCP 1.0 to be landed on gaia master so that they can test, the new features in music app for AVRCP 1.0 will be implemented and traced by this bug.
Depends on: 834554
Depends on: 900348

There will be a new webapi called "Inter App Communication" instead of MediaRemoteControl api, and we probably should consider to use it to handle the AVRCP commands.

The commands for AVRCP 1.0 are all handled and can interact with the music app in my test branch, but before landing it, the code must be cleaned up and some unit tests will be probably needed.
Whiteboard: [u= c= p=0]
Whiteboard: [u= c= p=0] → [u=devices c=BT p=0]
Depends on: 905595
Summary: [Music] Support bluetooth AVRCP 1.0 → [Music] Support bluetooth AVRCP 1.0 + 1.3
Duplicate of this bug: 905597
Duplicate of this bug: 905600
Blocks: 911921
This bug now covers the integration of AVRCP 1.0 + 1.3 and will be landed together.

This patch basically implemented the remote media controls with AVRCP commands enabled that I proposed, I am requesting a feedback from you because it's better that you can take a look on it first then give some feedbacks before I finalize it, such as how you think we can modify to adapt your design of the music controls in system app, after that, I will request for a formal review and you can review in detail.
Attachment #798963 - Flags: feedback?(squibblyflabbetydoo)
Comment on attachment 798963 [details]
Support bluetooth AVRCP 1.0 + 1.3

This generally seems good. I have a bunch of minor comments about the API. It might also make sense to have a few other people who like designing APIs take a look at this. Since we're putting this in /shared/js/, we definitely want to make sure we make the API as easy to use as possible so that other people* can take advantage of it without too much trouble.

* Probably just media team people for now, but I expect third parties will want to use this if they make their own media apps.
Attachment #798963 - Flags: feedback?(squibblyflabbetydoo) → feedback+
blocking-b2g: --- → koi+
Comment on attachment 798963 [details]
Support bluetooth AVRCP 1.0 + 1.3


Okay, I think I have addressed all the issues you mentioned on github, now the modified media remote controls should fit your code on the IAC part, before I finished rewriting the unit tests, can you please take a look on it again? hope I can make it before you go to Oslo, I will ping you again after I done the tests, thanks.


This patch is the media remote controls that I mentioned in the email, I have discussed with Jim and came out this design pattern, and we both think it will be nice that you can also have a look on it, can you also give us some feedback if you are probably available? thanks.
Attachment #798963 - Flags: review?(squibblyflabbetydoo)
Attachment #798963 - Flags: feedback?(dflanagan)
Attachment #798963 - Flags: feedback+
I finished rewriting the unit tests, basically I tested all the commands from AVRCP and IAC, and make sure they all fired to the command handlers and the received commands in events are correct.
Thanks to Shawn's help, we got a fantastic demo video!

Jim, while you are reviewing, if you don't have a bluetooth device that supports AVRCP 1.0 or 1.3, you can see how the patch actually works and interacts with the bluetooth car kit, hope this helps on the review.(We used the car kit's remote control to demo because we don't want the fingers to cover the display screen)
Comment on attachment 798963 [details]
Support bluetooth AVRCP 1.0 + 1.3

This is looking pretty good! I only have a couple of minor comments on Github about this. r- for now just because some of the changes might be subtle, and I want to take a quick look after you make them, but this is really close now. It should be a good basis for the IAC code I'll be landing soon.
Attachment #798963 - Flags: review?(squibblyflabbetydoo) → review-
Comment on attachment 798963 [details]
Support bluetooth AVRCP 1.0 + 1.3

Okay, I think I got all the issues addressed(For those I couldn't I have commented why) and this version should be good to go. Jim, can you please take another look on the patch? I hope everything looks good to you, thanks!
Attachment #798963 - Flags: review- → review?(squibblyflabbetydoo)
Comment on attachment 798963 [details]
Support bluetooth AVRCP 1.0 + 1.3

This looks good, and the unit tests pass. Unfortunately, I can't test it for real, since I lack a Bluetooth remote, but the video is good enough for me!
Attachment #798963 - Flags: review?(squibblyflabbetydoo) → review+
Thanks Jim! really appreciate all the reviews and suggestions.

Landed on master: ebbb325958c66c3e68e1f190472d5f6e9076d83a
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 843436
Comment on attachment 798963 [details]
Support bluetooth AVRCP 1.0 + 1.3

Cancelling the feedback request since the patch is landed.
Attachment #798963 - Flags: feedback?(dflanagan)
See Also: → 957508
You need to log in before you can comment on or make changes to this bug.