961984 - Music shall handle play-pause avrcp key event if music launches at the first time

Status: VERIFIED FIXED

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

Description

[Eric Chang [:ericcc] [:echang]]

Device: Nexus-5
Build: master build
Gaia defe57f89ef5f6d51a6d0ac0815ff6ce0367b50d
Gecko 82a055c447b89f1db919a0f617e7a8b39f6ef584
BuildID 20140117190211
Version 29.0a1

1. Connect Nexus 5 to an AVRCP supported device, a car kit in this case.
2. Start playing music.
3. Music played through the car kit.
4. Play/Pause working on phone.
5. Play/Pause not working on car kit.

Comment 1

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

I wonder that JB4.3 + 1.3 has the same problem??

Comment 2

[Eric Chang [:ericcc] [:echang]]

Nexus 4 + v1.3 - not stable, FF/REV/PLAY/PAUSE sometimes failed, mostly okay. 
https://pvtbuilds.mozilla.org/pvt/mozilla.org/b2gotoro/nightly/mozilla-central-mako/2013/12/2013-12-09-05-34-02/

Comment 3

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

But follow the same steps, on KK, bug is 100% reproducible?

Comment 4

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

After checking this bug. This bug is not related to KK specific bug, but it's bluetooth profile AVRCP 1.3 problem (bluedroid). I tested with Pioneer AVH-X5550BT. The problem is related to AVRCP 1.3 metadata notification.

Comment 5

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

Attachment: btsnoop_hci.log for avrcp 1.3 with AVH-x5550bt car kit

Comment 6

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

Hi Eric,
I can see sometimes car kit status bar is not sync, but i did not hit the avrcp command is not responding. Can you confirm this bug again? I guess the older build, the media player (I guess stage play/pause is not correct. Thanks.

Comment 7

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

Sorry. I find the way to reproduce the bug on nexus5.

Comment 8

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

STR:
1. Connect with AVH-X5550BT
2. AVH-X5550BT would automatically send play command once AVRCP get connected
3. Music player cannot be launched automatically

Note: 
1.bluedroid stack seems indeed send out key event.
2. If Music player launched and play music first and then connect with AVH-X5550BT, AVRCP play/pause can work normally.

Comment 9

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

> Note: 
> 1.bluedroid stack seems indeed send out key event.
> 2. If Music player launched and play music first and then connect with
> AVH-X5550BT, AVRCP play/pause can work normally.
I did some further clarification here:
If Music player launched without selecting songs and play, then connect with AVH-X5550BT, AVRCP won't work.
If Music player launched and play, then connect with AVH-X5550BT, AVRCP can work.

Comment 10

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

I further checked shell.js broadcasts media-button event, but I saw error from music player.

02-24 03:54:33.662 I/GeckoDump( 2264): !!!!!! avrcp media button!!!!!!!!!!!!!
02-24 03:54:33.802 E/GeckoConsole( 2911): [JavaScript Warning: "Unknown property 'will-change'.  Declaration dropped." {file: "app://music.gaiamobile.org/style/music.css" line: 293 column: 13 source: "  will-change: scroll-position;"}]
02-24 03:54:33.802 E/GeckoConsole( 2911): [JavaScript Warning: "Unknown property 'will-change'.  Declaration dropped." {file: "app://music.gaiamobile.org/style/music.css" line: 357 column: 13 source: "  will-change: scroll-position;"}]
02-24 03:54:33.972 E/GeckoConsole( 2911): [JavaScript Error: "PlayerView is not defined" {file: "app://music.gaiamobile.org/js/communications.js" line: 45}]
02-24 03:54:33.982 E/GeckoConsole( 2911): [JavaScript Error: "NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: [JavaScript Error: "PlayerView is not defined" {file: "app://music.gaiamobile.org/js/communications.js" line: 45}]'[JavaScript Error: "PlayerView is not defined" {file: "app://music.gaiamobile.org/js/communications.js" line: 45}]' when calling method: [nsIDOMSystemMessageCallback::handleMessage]" {file: "jar:file:///system/b2g/omni.ja!/components/SystemMessageManager.js" line: 96}]

Comment 11

[Dominic Kuo [:dkuo]]

Attachment: Check the play status then play for both play and playpause commands

Hey Shawn, as we discussed offline, the PlayerView probably was not ready when receiving the playpause command from the remote BT device, would you please try this WIP first? thanks.

Comment 12

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

Music player seems still cannot be launched via the very first play-pause-button press/release.

02-24 05:40:04.942 I/GeckoDump( 5639): !!!!!! avrcp media button!!!!!!!!!!!!!
02-24 05:40:04.952 I/Gecko   ( 5639): [Parent 5639] WARNING: waitpid failed pid:6460 errno:10: file ../../../gecko/ipc/chromium/src/base/process_util_posix.cc, line 254
02-24 05:40:04.952 I/Gonk    ( 5639): Setting nice for pid 6460 to 7
02-24 05:40:04.952 I/Gonk    ( 5639): Changed nice for pid 6460 from 1 to 7.
02-24 05:40:04.972 I/GeckoDump( 5639): !!!!!! avrcp media button!!!!!!!!!!!!!
02-24 05:40:05.012 E/GeckoConsole( 6460): [JavaScript Warning: "No meta-viewport tag found. Please explicitly specify one to prevent unexpected behavioural changes in future versions. For more help https://developer.mozilla.org/en/docs/Mozilla/Mobile/Viewport_meta_tag" {file: "app://music.gaiamobile.org/index.html#mix" line: 0}]
02-24 05:40:05.052 E/GeckoConsole( 6460): [JavaScript Warning: "Unknown property 'will-change'.  Declaration dropped." {file: "app://music.gaiamobile.org/style/music.css" line: 293 column: 13 source: "  will-change: scroll-position;"}]
02-24 05:40:05.052 E/GeckoConsole( 6460): [JavaScript Warning: "Unknown property 'will-change'.  Declaration dropped." {file: "app://music.gaiamobile.org/style/music.css" line: 357 column: 13 source: "  will-change: scroll-position;"}]
02-24 05:40:05.232 I/GeckoDump( 6460): MediaRemoteControls: media-play-pause-button-press
02-24 05:40:05.232 I/GeckoDump( 6460): MediaRemoteControls: media-play-pause-button-release

Comment 13

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

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #12)
> Music player seems still cannot be launched via the very first
> play-pause-button press/release.
> 
I also found if launching Music but stay at main menu, instead of going into Play view. Music application will not play.

Comment 14

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

I have tested play-pause key patch for Music. It seems works. So change this bug category to Music application.

Comment 15

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Set target milestone for this is included for 2/28 set.

Comment 16

[Dominic Kuo [:dkuo]]

Comment on attachment 8380552 [details] [review]
Check the play status then play for both play and playpause commands

Jim,

This issue is caused by the different bluetooth stacks, it seems the Bluedroid stack(in kitkat) will send the |playpause| command instead of the |play| command after A2DP is first connected and try to play the music immediately. The music communication expected to receive play and in the playpause function we didn't handle the condition that PlayerView is not ready, to fix this I have wrapped the logic for the play command into a function, for play and playpause we will both do the same logic, would you please review it? thanks.

Shawn,

I have addressed the issues we found and the patch should be good to go, would you please test it again on nexus 5 to make sure it does solve the problems? thanks.

Comment 17

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

I tested again your patch. It works.

Comment 18

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

Comment on attachment 8380552 [details] [review]
Check the play status then play for both play and playpause commands

I tested the patch, it works with car kit.

Comment 19

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

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #18)
> Comment on attachment 8380552 [details] [review]
> Check the play status then play for both play and playpause commands
> 
> I tested the patch, it works with car kit.

I found 'Music paused during the call' text always shows up after apply this patch. Please check again.

Comment 20

[Dominic Kuo [:dkuo]]

(In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #19)
> (In reply to Shawn Huang [:shuang] [:shawnjohnjr] from comment #18)
> > Comment on attachment 8380552 [details] [review]
> > Check the play status then play for both play and playpause commands
> > 
> > I tested the patch, it works with car kit.
> 
> I found 'Music paused during the call' text always shows up after apply this
> patch. Please check again.

Thanks Shawn, I will check my patch and see what's going on here, I am not going to cancel the review because we can catch Jim's attention first while I am finding the issue you mentioned.

Comment 21

[Jim Porter (:squib)]

Comment on attachment 8380552 [details] [review]
Check the play status then play for both play and playpause commands

r=me with my comments.

It always seemed a bit weird to me that "play" and "playpause" meant the same thing, though. I'd expect "play" to mean "if we're paused, start playing; if we're playing, do nothing". Oh well.

Comment 22

[Jim Porter (:squib)]

Also, make sure you fix comment 19, but you already know that. :)

Comment 23

[Dominic Kuo [:dkuo]]

(In reply to Jim Porter (:squib) from comment #21)
> Comment on attachment 8380552 [details] [review]
> Check the play status then play for both play and playpause commands
> 
> r=me with my comments.
> 
> It always seemed a bit weird to me that "play" and "playpause" meant the
> same thing, though. I'd expect "play" to mean "if we're paused, start
> playing; if we're playing, do nothing". Oh well.

Thanks for the review Jim, I don't know the exact reason why Bluedroid convert the |play| command to |playpause|, but as I commented in the patch, I believe Bluedroid is trying to be more compatible for all the bluetooth devices in the market, because some of them do not keep the play status, they will just send play or playpause via AVRCP requesting the player to play, so it just convert the commands for "please play my music" to playpause from the stack side, no matter it's |play| or |playpause|.

(In reply to Jim Porter (:squib) from comment #22)
> Also, make sure you fix comment 19, but you already know that. :)

I have found the issue in comment 19 and addressed in https://github.com/dominickuo/gaia/commit/c0a9791638d1e737957805dcbc96be363db67689, and I and doing the final check to make sure I didn't break anything, will land the patch later.

Comment 24

[Dominic Kuo [:dkuo]]

Update:

I have also added the tests and waiting for the travis to get green.

Comment 25

[Dominic Kuo [:dkuo]]

Landed on master: 1a47899c90212669b1b3ac0f067f181f2db3ba64

Comment 26

[Eric Chang [:ericcc] [:echang]]

Thansk, working fine with these 2 devices.

Nexus 4 - JB - v1.4
Gaia      a62c8df80732f9b640bfd90add5cdce71a39105c  │  
Gecko     cb6ce085e9672073c276a07bd6b1323c394d4c76  │  
BuildID   20140307043054                            │  
Version   30.0a1  

Buri v1.4
Gaia      98cf46d6623b164845fe1fdc99a2a7bf64aa667d                         │
Gecko     https://hg.mozilla.org/mozilla-central/rev/8095b7dd8f58          │
BuildID   20140306160203                                                   │
Version   30.0a1 ]

Comment 27

[Warren Holdsworth]

1.4 tested using Arctic P253BT Blue tooth headset.
Headset was able to play, pause, stop, forward, backward and volume up/down.

1.4 Environmental Variables:
Device: Buri 1.4 MOZ
BuildID: 20140527000202
Gaia: 0542778892a294d224e75af4a76be5d42938bc90
Gecko: d583ae109f54
Version: 30.0
Firmware Version: v1.2-device.cfg