[Music] [Bluetooth] support BT APIv2 in remote controls

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: gasolin, Assigned: gasolin)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.2 wontfix, b2g-master affected)

Details

Attachments

(2 attachments)

(Assignee)

Description

4 years ago
shared/js/media/remote_controls.js also use bluetooth functionality in music app.

we could use bluetooth_helper to keep bluetooth api call isolated from app.
Created attachment 8597305 [details] [review]
[gaia] gasolin:issue-1158192-2 > mozilla-b2g:master
(Assignee)

Comment 2

4 years ago
Comment on attachment 8597305 [details] [review]
[gaia] gasolin:issue-1158192-2 > mozilla-b2g:master

Since remote control is used by music app, we can use bluetoothAdapter to abstract the bluetooth call between API difference.

I test the patch with BT v1/v2 devices and all work fine. Please take a look and raise your consideration if this is a proper approach.
Attachment #8597305 - Flags: feedback?(iliu)
Attachment #8597305 - Flags: feedback?(dkuo)
(Assignee)

Updated

4 years ago
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Comment on attachment 8597305 [details] [review]
[gaia] gasolin:issue-1158192-2 > mozilla-b2g:master

Feedback+ with me. Since we have to support two API versions, it's better to move Bluetooth relative work out off app's controller. These version detection and interface(adapter) wrapper is done in bluetooth_helper.js already.
Attachment #8597305 - Flags: feedback?(iliu) → feedback+
(Assignee)

Comment 4

4 years ago
Comment on attachment 8597305 [details] [review]
[gaia] gasolin:issue-1158192-2 > mozilla-b2g:master

Test added for both remoteControl & BluetoothHelper. test fine on devices.
Attachment #8597305 - Flags: review?(iliu)
Attachment #8597305 - Flags: review?(dkuo)
Attachment #8597305 - Flags: feedback?(dkuo)

Comment 5

4 years ago
This should only goes to master, correct me if I am wrong, thanks.
status-b2g-v2.2: --- → wontfix
status-b2g-master: --- → affected
(Assignee)

Comment 6

4 years ago
yes, it should only goes to master
Comment on attachment 8597305 [details] [review]
[gaia] gasolin:issue-1158192-2 > mozilla-b2g:master

Besides of some nits addressed on GitHub, the patch is good for me. After patch updated, I'm happy to review it again. Thanks:)
Attachment #8597305 - Flags: review?(iliu)

Comment 8

4 years ago
Fred, thanks for working on this, before I actually review the patch, I saw you included one js file in index.html. To make sure we didn't slow down the startup times, would you please use Raptor[1] to test your patch then paste the before/after results here? (see the setup we used in Bug 1153985 comment 3)

We do this for reason because recently we found performance regression from 2.1 to 2.2(Bug 1153985), so I hope for the new patches we don't break it again, or it's really hard to find the root causes after we landed many new patches.

[1] https://developer.mozilla.org/en-US/Firefox_OS/Automated_testing/Raptor
Flags: needinfo?(gasolin)
(Assignee)

Comment 9

4 years ago
thanks for the instruction,

master 1bf37ff2a500f91249b51cb986aace60a397ccb9

Metric Mean Median Min Max StdDev p95
-------------------------------- -------- -------- -------- -------- ------ ---
coldlaunch.navigationLoaded 2482.000 2482.000 2482.000 2482.000 0.000 n/a
coldlaunch.navigationInteractive 2637.000 2637.000 2637.000 2637.000 0.000 n/a
coldlaunch.visuallyLoaded 2767.000 2767.000 2767.000 2767.000 0.000 n/a
coldlaunch.contentInteractive 2768.000 2768.000 2768.000 2768.000 0.000 n/a
coldlaunch.fullyLoaded 3651.000 3651.000 3651.000 3651.000 0.000 n/a
coldlaunch.pss 23.200 23.200 23.200 23.200 0.000 n/a
coldlaunch.uss 17.800 17.800 17.800 17.800 0.000 n/a
coldlaunch.rss 38.200 38.200 38.200 38.200 0.000 n/a

master with this patch

Metric Mean Median Min Max StdDev p95
-------------------------------- -------- -------- -------- -------- ------ ---
coldlaunch.navigationLoaded 2360.000 2360.000 2360.000 2360.000 0.000 n/a
coldlaunch.navigationInteractive 2503.000 2503.000 2503.000 2503.000 0.000 n/a
coldlaunch.visuallyLoaded 2618.000 2618.000 2618.000 2618.000 0.000 n/a
coldlaunch.contentInteractive 2618.000 2618.000 2618.000 2618.000 0.000 n/a
coldlaunch.fullyLoaded 3525.000 3525.000 3525.000 3525.000 0.000 n/a
coldlaunch.rss 38.200 38.200 38.200 38.200 0.000 n/a
coldlaunch.uss 17.800 17.800 17.800 17.800 0.000 n/a
coldlaunch.pss 23.200 23.200 23.200 23.200 0.000 n/a
Flags: needinfo?(gasolin)
(Assignee)

Comment 10

4 years ago
Created attachment 8599239 [details]
rapter_master_result.png

FYR result on master
(Assignee)

Comment 11

4 years ago
If I intercept the result correctly, the patch improve about 100ms for performance.
(Assignee)

Comment 12

4 years ago
Possible reason for performance gain: 

BluetoothHelper instance try to retrieve adapter when any of its method is first called, so the initial procedure in remote control doesn't do any real operation until any related method is called. 

https://github.com/gasolin/gaia/blob/master/shared/js/bluetooth_helper.js
(Assignee)

Comment 13

4 years ago
Re-run with same base commit, result:

master

MacBook-Air:gaia:% APP=music node tests/raptor/launch_test RUNS=30   <master ✗>
[Raptor] Preparing to start testing...
[Cold Launch: Music] Priming application
[Cold Launch: Music] Starting run 1
[Cold Launch: Music] Run 1 complete

Metric                            Mean      Median    Min       Max       StdDev  p95
--------------------------------  --------  --------  --------  --------  ------  ---
coldlaunch.navigationLoaded       2523.000  2523.000  2523.000  2523.000  0.000   n/a
coldlaunch.navigationInteractive  2656.000  2656.000  2656.000  2656.000  0.000   n/a
coldlaunch.visuallyLoaded         2813.000  2813.000  2813.000  2813.000  0.000   n/a
coldlaunch.contentInteractive     2814.000  2814.000  2814.000  2814.000  0.000   n/a
coldlaunch.fullyLoaded            3719.000  3719.000  3719.000  3719.000  0.000   n/a
coldlaunch.pss                    23.300    23.300    23.300    23.300    0.000   n/a
coldlaunch.uss                    17.800    17.800    17.800    17.800    0.000   n/a
coldlaunch.rss                    38.400    38.400    38.400    38.400    0.000   n/a

[Cold Launch: Music] Testing complete

with patch

MacBook-Air:gaia:% APP=music node tests/raptor/launch_test RUNS=30
[Raptor] Preparing to start testing...
[Cold Launch: Music] Priming application
[Cold Launch: Music] Starting run 1
[Cold Launch: Music] Run 1 complete

Metric                            Mean      Median    Min       Max       StdDev  p95
--------------------------------  --------  --------  --------  --------  ------  ---
coldlaunch.navigationLoaded       2516.000  2516.000  2516.000  2516.000  0.000   n/a
coldlaunch.navigationInteractive  2655.000  2655.000  2655.000  2655.000  0.000   n/a
coldlaunch.visuallyLoaded         2793.000  2793.000  2793.000  2793.000  0.000   n/a
coldlaunch.contentInteractive     2793.000  2793.000  2793.000  2793.000  0.000   n/a
coldlaunch.fullyLoaded            3658.000  3658.000  3658.000  3658.000  0.000   n/a
coldlaunch.rss                    38.600    38.600    38.600    38.600    0.000   n/a
coldlaunch.pss                    23.500    23.500    23.500    23.500    0.000   n/a
coldlaunch.uss                    17.900    17.900    17.900    17.900    0.000   n/a

[Cold Launch: Music] Testing complete

Comment 14

4 years ago
Thanks Fred, the results looks good and it seems not impacting the startup times, it should be expected because the bluetooth helper does similar works that we did for bluetooth in the remote controls. However, music app is planning to lazy load the remote controls module, it means we should do it for the bluetooth helper as well, so after this landed, we need a followup to lazy load the bluetooth helper.
(Assignee)

Updated

4 years ago
Blocks: 1160067

Comment 15

4 years ago
Comment on attachment 8597305 [details] [review]
[gaia] gasolin:issue-1158192-2 > mozilla-b2g:master

Fred, thanks for working on this, overall the patch looks good to me. For the remote controls part, I have several comments but mostly they are about to follow the original coding style, please address them before landing it.

For the bluetooth helper part, Ian should be more familiar with it so I will leave it to him.
Attachment #8597305 - Flags: review?(dkuo) → review+
(Assignee)

Comment 16

4 years ago
Comment on attachment 8597305 [details] [review]
[gaia] gasolin:issue-1158192-2 > mozilla-b2g:master

Thanks for review.

I add ondisable state handler in bluetooth helper and the test shows no difference between has/without that handler. Please kindly review it again.
Attachment #8597305 - Flags: review?(iliu)
Comment on attachment 8597305 [details] [review]
[gaia] gasolin:issue-1158192-2 > mozilla-b2g:master

The patch is working fine on callscreen, music apps while they are also using Bluetooth headset. And the pairing process is normal as before. r+ with me. Thanks.
Attachment #8597305 - Flags: review?(iliu) → review+
(Assignee)

Comment 18

4 years ago
merged https://github.com/mozilla-b2g/gaia/commit/02d667c78e9a6a627fd891614053938cf91ed3c4

thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Updated

4 years ago
Blocks: 1167062
You need to log in before you can comment on or make changes to this bug.