834554 - [b2g-bluetooth] Support Bluetooth AVRCP 1.0

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

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

Add Media button (play, pause, stop, forward, backward) support. In order to enable Bluetooth AVRCP (Audio / Video Remote Control Profile) profile. It required to enable key mapping. This method of overriding VK_F15-F16, VK_U-Z and sending a system message.

Comment 2

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

Attachment: Add support for media button

Comment 3

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

Hi Eric,
Similar bug is Bug 805744. I just added at the same place.

Comment 4

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Comment on attachment 706296 [details] [diff] [review]
Add support for media button

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

Just curious, where's the keymappings come from? Are you just using unused keys?

Comment 5

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

Hi Kyle,
It's because BlueZ AVRCP uses these key for mapping.

In AVRCP.kl

key 200   MEDIA_PLAY          WAKE
key 201   MEDIA_PAUSE         WAKE
key 166   MEDIA_STOP          WAKE
key 163   MEDIA_NEXT          WAKE
key 165   MEDIA_PREVIOUS      WAKE
key 168   MEDIA_REWIND        WAKE
key 208   MEDIA_FAST_FORWARD  WAKE


In BlueZ stack audio/control.c
 } key_map[] = {
          { "PLAY",               PLAY_OP,                KEY_PLAYCD },
          { "STOP",               STOP_OP,                KEY_STOPCD },
          { "PAUSE",              PAUSE_OP,               KEY_PAUSECD },
          { "FORWARD",            FORWARD_OP,             KEY_NEXTSONG },
          { "BACKWARD",           BACKWARD_OP,            KEY_PREVIOUSSONG },
          { "REWIND",             REWIND_OP,              KEY_REWIND },
          { "FAST FORWARD",       FAST_FORWARD_OP,        KEY_FASTFORWARD },
          { NULL }
  };

Comment 6

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

Ah, ok, figured it was that, just wanted to make sure in case anyone asks me (I still get lots of bluetooth questions, heh).

Comment 7

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 706296 [details] [diff] [review]
Add support for media button

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

Looks good to me.

Comment 8

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

Attachment: Add media button support

Comment 9

[Mozilla RelEng Bot]

Try run for ff15ad52ecb8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=ff15ad52ecb8
Results (out of 18 total builds):
    success: 17
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-ff15ad52ecb8

Comment 10

[Mozilla RelEng Bot]

Try run for f9f3b50e8191 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=f9f3b50e8191
Results (out of 14 total builds):
    success: 14
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-f9f3b50e8191

Comment 11

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

Attachment: Add Media button support for Bluetooth AVRCP 1.3

Update patch title and reviewer

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 716392 [details] [diff] [review]
Add Media button support for Bluetooth AVRCP 1.3

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

::: b2g/chrome/content/shell.js
@@ +410,5 @@
>        this.lastHardwareButtonEventType = type;
>        gSystemMessenger.broadcastMessage('headset-button', type);
>        return;
>      }
> +    if (((evt.keyCode >= evt.DOM_VK_U && evt.keyCode <= evt.DOM_VK_Z) ||

Nit: blank line before the if() and add a comment explaining why we do that.

::: widget/gonk/GonkKeyMapping.h
@@ +107,5 @@
> +    NS_VK_V, // MEDIA_STOP
> +    NS_VK_W, // MEDIA_NEXT
> +    NS_VK_X, // MEDIA_PREVIOUS
> +    NS_VK_Y, // MEDIA_REWIND
> +    NS_VK_Z, // MEDIA_FAST_FORWARD

So if I hit "V" on a real keyboard this will trigger a media-stop-button system message?

Comment 13

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

I checked nsIDOMKeyEvent.idl, DOM_VK_V defines as 0x56, which is Key code constant: Stop media key. In the original GonkKeyMapping.h, NS_VK_U commented as MEDIA_PLAY_PAUSE. NS_VK_V looks weired but it maps to Media key instead of "V".

Comment 14

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

Sorry, now I understand your question. I think I'm wrong about mapping. I will change it.

Comment 15

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

Hi Fabrice,
I misunderstood key code and virtual key code, this is why I misused NS_VK_V. I think the proper way is to add NS_VK_MEDIA_STOP virtual key.
I found actually there is no Media key relative in nsIDOMKeyEvent.idl. I wonder it is ok to add media button definition into nsIDOMKeyEvent.idl. 
If it is NOT proper to add media button in nsIDOMKeyEvent.idl.
Can we use other keys like Bug 805744 (use F1)?
Can you provide any suggestion?

Comment 16

[[:fabrice] Fabrice Desré]

Shawn, we need input from people that are doing DOM3 Events here I think, see bug 674739.

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Please wait the fix of bug 842927. Then, you can get rid of such hacky implementation.

Comment 18

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

Hi Masayuki, any target schedule regarding the fix of bug 842927? Will it be part of v1.1?

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Shawn Huang from comment #18)
> Hi Masayuki, any target schedule regarding the fix of bug 842927? Will it be
> part of v1.1?

I'd like to land them ASAP, but the schedule depends on the reviewers. First, smaug will check all of them. After that, each widget reviewer will review them.

Comment 20

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

Due to schedule difficulty for A2DP/AVRCP, can we accept hacky implementation first?

Comment 21

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

Attachment: Bug 834554-Add media button support for AVRCP 1.3 feature

I changed to other function keys from F15~F22

Comment 22

[[:fabrice] Fabrice Desré]

Comment on attachment 730596 [details] [diff] [review]
Bug 834554-Add media button support for AVRCP 1.3 feature

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

I'm sorry but I don't see any reason to rush this before bug 842927 lands.

Comment 23

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Now, these keys are available with KeyboardEvent.key or nsKeyEvent::mKeyNameIndex. See https://developer.mozilla.org/en-US/docs/DOM/KeyboardEvent#keyname_table_android for the mapping.

Comment 24

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

OK. I will follow the new way to implement AVRCP for Android, thank you.

Comment 25

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

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (Offline 5/28 - 6/1) from comment #23)
> Now, these keys are available with KeyboardEvent.key or
> nsKeyEvent::mKeyNameIndex. See
> https://developer.mozilla.org/en-US/docs/DOM/
> KeyboardEvent#keyname_table_android for the mapping.
I'm not an expert for this keyboard event.
There are a few questions I want to know further based on my original patch.
In b2g/chrome/content/shell.js, we use keyCode to send system message,
which binds android keycode in widget/gonk/GonkKeyMapping.h.

My question is from current implementation, I still cannot use virtual key of media button series keycode. I checked dom/interfaces/events/nsIDOMKeyEvent.idl, I don't see extra virtual key definition defined. Would you mind to provide me some more advices here?

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Are you trying to improve here?
http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#360

If so, use KeyboardEvent.key instead of KeyboardEvent.keyCode.
http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-KeyboardEvent-key

Comment 27

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

Yes. It works fine on Android.
Thanks a lot.

Comment 28

[Gina Yeh [:gyeh] [:ginayeh]]

Hi masayuki,

When trying to map all media-related keys in shell.js, and I found something interesting. All media-related keys are mapping to a name starting with MediaXxxxx. For example, MediaPlay, MediaPause, and MediaRewind.

Just out of curiosity. There's one exception: FastFwd. In widget/shared/NativeKeyToDOMKeyName.h:1048:KEY_MAP_ANDROID (FastFwd, AKEYCODE_MEDIA_FAST_FORWARD), are we mapping |AKEYCODE_MEDIA_FAST_FORWARD| to |FastFwd| rather than to |MediaFastFwd| for other reason?

Comment 29

[Gina Yeh [:gyeh] [:ginayeh]]

I'd like to steal it from Shawn :)

Comment 30

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v1): Support AVRCP 1.0

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Gina Yeh [:gyeh] [:ginayeh] from comment #28)
> Hi masayuki,
> 
> When trying to map all media-related keys in shell.js, and I found something
> interesting. All media-related keys are mapping to a name starting with
> MediaXxxxx. For example, MediaPlay, MediaPause, and MediaRewind.
> 
> Just out of curiosity. There's one exception: FastFwd. In
> widget/shared/NativeKeyToDOMKeyName.h:1048:KEY_MAP_ANDROID (FastFwd,
> AKEYCODE_MEDIA_FAST_FORWARD), are we mapping |AKEYCODE_MEDIA_FAST_FORWARD|
> to |FastFwd| rather than to |MediaFastFwd| for other reason?

We just use the key names which defined by D3E draft. If the names will be changed, we need to change them.

FYI: https://www.w3.org/Bugs/Public/show_bug.cgi?id=22084

Comment 32

[Gina Yeh [:gyeh] [:ginayeh]]

Thanks for your information, masayuki. :)

Comment 33

[Eric Chou [:ericchou] [:echou]]

Comment on attachment 758487 [details] [diff] [review]
Patch 1(v1): Support AVRCP 1.0

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

I'm ok with the overall idea, though I'm not the most suitable person to do this since there's not much related to Bluetooth. ;)

Please redirect to Fabrice after revision. Thanks!

::: b2g/chrome/content/shell.js
@@ +382,5 @@
>          break;
> +    }
> +
> +    // Handle media-related keys and set mediaKey to true
> +    var isMediaKey = false;

To avoid adding "isMediaKey = true;" in each 'case' block, you could set it to false in the 'default' case.

@@ +384,5 @@
> +
> +    // Handle media-related keys and set mediaKey to true
> +    var isMediaKey = false;
> +    switch (evt.key) {
> +      case 'MediaNextTrack':     // Media backward button

Weird comment. Shouldn't it be 'forward' button?

@@ +409,5 @@
> +        type = 'media-stop-button';
> +        isMediaKey = true;
> +        break;
> +      case 'MediaRewind':        // Media rewind button
> +        type = 'media-rewind';

To be consistent, please use 'media-rewind-button' instead.

@@ +416,5 @@
> +      case 'FastFwd':            // Media fast forward button
> +        type = 'media-fast-forward-button';
> +        isMediaKey = true;
> +        break;
> +    }

About those comments at the end of each 'case' statement, since the name of these keys are clear enough and almost the same as what you're trying to explain in comments, we don't really need them.

@@ +418,5 @@
> +        isMediaKey = true;
> +        break;
> +    }
> +
> +    // A real key. Don't filter it al all; let it propagate to Gaia

Please refine the comment.

@@ +420,5 @@
> +    }
> +
> +    // A real key. Don't filter it al all; let it propagate to Gaia
> +    if (!type) {
> +      return;

We can move this to 'default' of above switch block.

Comment 34

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 758487 [details] [diff] [review]
Patch 1(v1): Support AVRCP 1.0

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

Thanks. Will update patch soon.

::: b2g/chrome/content/shell.js
@@ +382,5 @@
>          break;
> +    }
> +
> +    // Handle media-related keys and set mediaKey to true
> +    var isMediaKey = false;

Good point. Thanks.

@@ +384,5 @@
> +
> +    // Handle media-related keys and set mediaKey to true
> +    var isMediaKey = false;
> +    switch (evt.key) {
> +      case 'MediaNextTrack':     // Media backward button

Oops! Will fix in the next patch.

@@ +388,5 @@
> +      case 'MediaNextTrack':     // Media backward button
> +        type = 'media-next-track-button';
> +        isMediaKey = true;
> +        break;
> +      case 'MediaPreviousTrack': // Media forward button

Should be "backward" button

@@ +420,5 @@
> +    }
> +
> +    // A real key. Don't filter it al all; let it propagate to Gaia
> +    if (!type) {
> +      return;

I'm afraid we couldn't move this to the default case of above switch since hardware key like "HOME" shouldn't be ignored.

Comment 35

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v2): Support AVRCP 1.0

Updated based on feedback from echou.

Comment 36

[Gina Yeh [:gyeh] [:ginayeh]]

Comment on attachment 761846 [details] [diff] [review]
Patch 1(v2): Support AVRCP 1.0

Fabrice, I'm not sure who should be the reviewer of this part. Could you help to review this patch or re-direct to to right reviewer. Thanks.

Comment 37

[[:fabrice] Fabrice Desré]

Comment on attachment 761846 [details] [diff] [review]
Patch 1(v2): Support AVRCP 1.0

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

::: b2g/chrome/content/shell.js
@@ +381,5 @@
>          type = 'headset-button';
>          break;
> +    }
> +
> +    var isMediaKey;

Nit: s/var/let

@@ +440,5 @@
>        gSystemMessenger.broadcastMessage('headset-button', type);
>        return;
>      }
>  
> +    if (isMediaKey) {

I don't see any place where isMediaKey is set to true.

Comment 38

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v3): Support AVRCP 1.0

Fabrice, thanks for your review. I update the patch and please check it.

Comment 39

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v4): Support AVRCP 1.0

Sorry for annoying review request. v2 and v3 don't work though :(

v4 has been tested and verified. Please help to check it. Thanks.

Comment 40

[[:fabrice] Fabrice Desré]

Comment on attachment 762450 [details] [diff] [review]
Patch 1(v4): Support AVRCP 1.0

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

::: b2g/chrome/content/shell.js
@@ +415,5 @@
> +      case 'FastFwd':
> +        type = 'media-fast-forward-button';
> +        isMediaKey = true;
> +        break;
> +    }

I think that we could something a bit nicer:
let mediaKeys = {"MediaNextTrack": "media-next-track-button", 
                 "MediaPreviousTrack": "media-previous-track-button",
                 ...};
let isMediaKey = false;
if (mediaKeys[evt.key) {
  type = mediaKeys[evt.key;
  isMediaKey = true;
}

What do you think?

Comment 41

[Gina Yeh [:gyeh] [:ginayeh]]

(In reply to Fabrice Desré [:fabrice] from comment #40)
> I think that we could something a bit nicer:
> let mediaKeys = {"MediaNextTrack": "media-next-track-button", 
>                  "MediaPreviousTrack": "media-previous-track-button",
>                  ...};
> let isMediaKey = false;
> if (mediaKeys[evt.key) {
>   type = mediaKeys[evt.key;
>   isMediaKey = true;
> }
> 
> What do you think?

It's much much better. Let me try it and update patch soon.

Comment 42

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Patch 1(v5): Support AVRCP 1.0

Thanks for your feedback. It looks neater :)

Comment 43

[Gina Yeh [:gyeh] [:ginayeh]]

Attachment: Final patch : Support Bluetooth AVRCP 1.0, r=fabrice

http://hg.mozilla.org/projects/birch/rev/37625e202839

Comment 44

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/37625e202839