ying.xu Assignee
	Description • 11 years ago

device tarako v1.3T branch

reproduce way:
open dialer app, click on figures of dialer panel
response is very slow

we can confirm that the tone is not the reason for this through the DS5 profiling result.

need to investigate this

	ying.xu Assignee
	Comment 1 • 11 years ago

User 86%, System 13%, IOW 0%, IRQ 0%
User 84 + Nice 183 + Sys 40 + Idle 0 + IOW 0 + IRQ 0 + SIRQ 0 = 307

  PID PR CPU% S  #THR     VSS     RSS PCY UID      Name
 1812  0  56% R    18  79196K  26872K  fg app_1812 /system/b2g/plugin-container
   81  0  31% S    51 156248K  35936K  fg root     /system/b2g/b2g

	Steven Yang [:styang]
	Comment 2 • 11 years ago

Triage to 1.3T+

	Gabriele Svelto [:gsvelto]
	Comment 3 • 11 years ago

Was this tested before or after bug 917193 landed? That bug should have improved the situation here IMHO.

	ying.xu Assignee
	Comment 4 • 11 years ago

(In reply to Gabriele Svelto [:gsvelto] from comment #3)
> Was this tested before or after bug 917193 landed? That bug should have
> improved the situation here IMHO.

bug 917193 has improved the situation actually, but not fix this.

	ying.xu Assignee
	Comment 5 • 11 years ago

And I noticed that deleting a figure by clicking on the 'X' button is very quick.

	Anthony Ricaud (:rik)
	Comment 6 • 11 years ago

I think bug 967440 can help here. It's not in my list of priorities though so if anyone wants to finish it, feel free to steal it from me.

	Joe Cheng [:jcheng] (please needinfo)
	Comment 7 • 11 years ago

QAWANTED to confirm latest status. thanks

	Brian Huang [:brianhuang][:brhuang]
	Comment 8 • 11 years ago

William, please help on this confirmation. If the keyboard response is acceptable, upload a video for partner.

	William Hsu [:whsu]
	Comment 9 • 11 years ago

Hi, Brian,

No problem. Thanks for the reminder.

-----------------------------------------------------------
Hi, all,

After I did the performance measurement, I think the current keypad response time of Tarako device is acceptable.
The keypad response time of Taroko device is  0.2362 second/key. (Build ID: 20140310)
The keypad response time of Buri device is  0.2124 second/key.
Compare with Buri device, the keypad response time of Tarako only increases 0.0238 second/key

My test scenario is as below.
Step 1. Launch the dialer app
Step 2. Try to input 10 numbers.
Step 3: Repeat 5 times

Here are the demo video.
Buri: https://dc1.safesync.com/LMtzcYMp/Buri_Dialer_20140310.wmv?a=WYfDsc0_NKc
Tarako: https://dc1.safesync.com/LMtzcYMp/Tarako_Dialer_20140310.wmv?a=74Stj9qirmo

Also, attach the test result and raw data. (Dialer_keypad_20140310.png)
Buri: 
01. 2.17 seconds (10 keys)
02. 2.13 seconds (10 keys)
03. 2.11 seconds (10 keys)
04. 2.07 seconds (10 keys)
05. 2.14 seconds (10 keys)

Tarako:
01. 2.36 seconds (10 keys)
02. 2.31 seconds (10 keys)
03. 2.26 seconds (10 keys)
04. 2.53 seconds (10 keys)
05. 2.35 seconds (10 keys)

* Build Info.:
 Buri:
 - Gaia      71f78f7f68fcf5e23cc5965fee0dad45289c438b
 - Gecko     https://hg.mozilla.org/mozilla-central/rev/5dc091b7e86f
 - BuildID   20140304160205
 - Version   30.0a1

 Tarako:
 - Gaia      7f434a8799b8c7b2b419ea118b2de7c2682c170d
 - Gecko     8da7b6293d63b140932893fed6a45f91bfa93631
 - BuildID   20140310075556
 - Version   28.0

------------------------------------------------------------------
So, I think we can close this bug.
Any thought? Thanks.

	Joe Cheng [:jcheng] (please needinfo)
	Comment 11 • 11 years ago

James, seem like we can resolve this bug as workforme, can you confirm on your side? thanks

	James Zhang (Spreadtrum)
	Comment 12 • 11 years ago

(In reply to Joe Cheng [:jcheng] from comment #11)
> James, seem like we can resolve this bug as workforme, can you confirm on
> your side? thanks

If close audio, it's still slow. 
Please compare it with android dialer.

	William Hsu [:whsu]
	Comment 13 • 11 years ago

Hi, James,

Here are my demo video.
- https://dc1.safesync.com/LMtzcYMp/Tarako_Dialer_20140310.wmv?a=74Stj9qirmo

I didn't see heavy delay while I did the test on the latest Tarako build.
No matter the keypad sound was turned on/off, the keypad worked as usual.

Could you please provide the 1) Build ID 2) Commit number?

Also, if possible, please provide the keypad response time of android platform.
Thanks for your help.

	James Zhang (Spreadtrum)
	Comment 14 • 11 years ago

(In reply to William Hsu [:whsu] from comment #13)
> Hi, James,
> 
> Here are my demo video.
> - https://dc1.safesync.com/LMtzcYMp/Tarako_Dialer_20140310.wmv?a=74Stj9qirmo
> 
> I didn't see heavy delay while I did the test on the latest Tarako build.
> No matter the keypad sound was turned on/off, the keypad worked as usual.
> 
> Could you please provide the 1) Build ID 2) Commit number?
> 
> Also, if possible, please provide the keypad response time of android
> platform.
> Thanks for your help.

Can you get android pac from kai-zhen.li and compare with your version?
Ying, do we need provide tarako DS5 profiling result?

	Joe Cheng [:jcheng] (please needinfo)
	Comment 15 • 11 years ago

back to 1.3T? as we are waiting further information

	Joe Cheng [:jcheng] (please needinfo)
	Comment 16 • 11 years ago

triage: minus for now. please nominate again if this bug still happens

	ying.xu Assignee
	Comment 17 • 11 years ago

After I disable the audio when clicking on the touch,
The response is highly improved.

diff --git a/apps/communications/dialer/js/tone_player.js b/apps/communications/dialer/js/tone_player.js
index b62bc89..ccefd62 100644
--- a/apps/communications/dialer/js/tone_player.js
+++ b/apps/communications/dialer/js/tone_player.js
@@ -131,9 +131,9 @@ var TonePlayer = {
 
   start: function tp_start(frequencies, shortPress) {
     if (shortPress) {
-      this._playSample(frequencies);
+   //   this._playSample(frequencies);
     } else {
-      this._startAt(frequencies, 0, shortPress ? kShortPressDuration : 0);
+   //   this._startAt(frequencies, 0, shortPress ? kShortPressDuration : 0);
     }
   },

tested by top
User 86%, System 10%, IOW 0%, IRQ 0%
User 272 + Nice 0 + Sys 32 + Idle 9 + IOW 0 + IRQ 0 + SIRQ 0 = 313

  PID PR CPU% S  #THR     VSS     RSS PCY UID      Name
22232  0  85% R    47 139900K  48012K  fg root     /system/b2g/b2g

	ying.xu Assignee
	Comment 18 • 11 years ago

another try

This patch also improved a lot.
Seems new audio() cuased audio driver in HAL and kernel took much CPU time.

diff --git a/apps/communications/dialer/js/tone_player.js b/apps/communications/dialer/js/tone_player.js
index b62bc89..ceb4aff 100644
--- a/apps/communications/dialer/js/tone_player.js
+++ b/apps/communications/dialer/js/tone_player.js
@@ -9,6 +9,7 @@ const kReleaseDuration = 0.05;
 
 var TonePlayer = {
   _audioContext: null,
+  _audioElement: null,
   _gainNode: null,
   _playingNodes: [],
   _tonesSamples: {
@@ -51,12 +52,16 @@ var TonePlayer = {
    */
   _playSample: function tp_playSample(frequencies) {
     var sample = null;
+    if(!this._audioElement)
+       this._audioElement = new Audio();
+
+    sample = this._audioElement;
 
     for (var i in this._tonesSamples) {
       if ((frequencies.length === 2) &&
           (frequencies[0] === this._tonesSamples[i][0]) &&
-          (frequencies[1] === this._tonesSamples[i][1])) {
-        sample = new Audio(i);
+          (frequencies[1] === this._tonesSamples[i][1])) {        
+        sample.src = i;
         break;
       }
     }

	Mike Lee [:mlee]
	Comment 19 • 11 years ago

Ying, did you want to submit an official patch for review?

	ying.xu Assignee
	Comment 21 • 11 years ago

The audio is still missing sometimes.
We know some problems in our audio drivers.We will fix that.

	Etienne Segonzac (:etienne)
	Comment 22 • 11 years ago

Comment on attachment 8392000 [details]
PR to review , based on v1.3t

Looking good but forwarding the review to Gabriele who did most of the performance work on this part of the dialer.

Also please note that we'll need to land the patch on master first.

	Gabriele Svelto [:gsvelto]
	Comment 23 • 11 years ago

Comment on attachment 8392000 [details]
PR to review , based on v1.3t

I've added a couple of nits on the GitHub PR but there's a fundamental issue with this patch that keeps me from r+'ing it and that I don't think it's fixable.

Whenever you set the |src| property it seems to me that the previous sound finishes playing and then the new one is played; this means that when typing really fast we skip some sounds. The samples are 120ms in length IIRC so this isn't a big issue and we might just live with it until we switch back to Web Audio proper, but I'd like to hear what :etienne thinks about this before r+'ing your patch.

Also you should provide a patch/PR against the master branch first, once we have that reviewed and landed we can uplift it here.

	Etienne Segonzac (:etienne)
	Comment 24 • 11 years ago

Not directly answering the question but: wouldn't bug 967440 help us a lot here?
If we can get the best user experience with acceptable cpu usage that's a win :)

	Gabriele Svelto [:gsvelto]
	Comment 25 • 11 years ago

(In reply to Etienne Segonzac (:etienne) from comment #24)
> Not directly answering the question but: wouldn't bug 967440 help us a lot here?
> If we can get the best user experience with acceptable cpu usage that's a win :)

It might, I'll try to give it a spin tomorrow on my Tarako and see if it helps. We've already got a workaround for this particular issue and the idea of having a second workaround on top of the first doesn't make me happy :-|

	Etienne Segonzac (:etienne)
	Comment 28 • 11 years ago

Comment on attachment 8392640 [details]
PR to review @master

forwarding to Gabriele and carrying my feedback+ since the gain in noticeable (a lot less key highlight loss etc...).

	Gabriele Svelto [:gsvelto]
	Comment 29 • 11 years ago

Comment on attachment 8392640 [details]
PR to review @master

r=me with the nits addressed, see my comment on GitHub.

	Gabriele Svelto [:gsvelto]
	Comment 30 • 11 years ago

Comment on attachment 8392641 [details]
PR to review @v1.3t

The patch is identical to the master one so let's cherry pick the change once we've landed it on master; we don't need a separate pull request for this.

	ying.xu Assignee
	Comment 31 • 11 years ago

update according to Comment 29  Gabriele Svelto [:gsvelto] 2014-03-19 08:39:27 PDT

	Gabriele Svelto [:gsvelto]
	Comment 32 • 11 years ago

Comment on attachment 8393959 [details]
PR to review@master

I had to restart the marionette_js test job and now Travis is green, ready to merge. Note that you don't need to ask for review again if I have already given you an r+ with nits, you just have to address the nits.

	Gabriele Svelto [:gsvelto]
	Comment 33 • 11 years ago

Merged to gaia/master 75cc0092a71d4a4e8ee2c81200ed60cf810f05e2

Nom'ing again as this might be needed for decent keypad tones on Tarako.

	Joe Cheng [:jcheng] (please needinfo)
	Comment 34 • 11 years ago

hi YingXu, how much improvement do you see here? lets understand the improvement here before we decide for tarako. thanks

	ying.xu Assignee
	Comment 35 • 11 years ago

(In reply to Joe Cheng [:jcheng] from comment #34)
> hi YingXu, how much improvement do you see here? lets understand the
> improvement here before we decide for tarako. thanks

very obvious improvement.

While the side effect is obvious, some of click tone will be missing if press the button very fast

	Joe Cheng [:jcheng] (please needinfo)
	Comment 36 • 11 years ago

triage: 1.3T+ to improve performance. can you please update it and resolved fixed on status-b2g-v1.3T? thanks

	[:fabrice] Fabrice Desré
	Comment 37 • 11 years ago

Why has this patch not been uplifted yet?

	ying.xu Assignee
	Comment 38 • 11 years ago

https://github.com/mozilla-b2g/gaia/commit/14ef4fcdf9199f04f7678755c917dc77f51e13ba

	Montoya Clemmons (MClemmons)
	Comment 39 • 11 years ago

Following STR from Comment 0, clicking on figures on keypad is in the acceptable range and doesn't give the perception of being slow on the latest 1.3T build. 

Environmental Variables:
Device: Tarako 1.3T MOZ
BuildID: 20140428014001
Gaia: 8895b180ed636069473703d0e7b73086989601ce
Gecko: 7caf4b5abfce
Version: 28.1
Firmware Version: sp6821a_gonk4.0_user.pac

	Jan Jongboom [:janjongboom] (inactive)
	Comment 40 • 10 years ago

Comment on attachment 8393959 [details]
PR to review@master

Request for uplift from partner for Dolphin 1.4.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Initial implementation
[User impact] if declined: Dialer sound is off on slow devices
[Testing completed]: Yes
[Risk to taking this patch] (and alternatives if risky): Small change, just caching the audio element
[String changes made]: None

	Preeti Raghunath(:Preeti)
	Comment 41 • 10 years ago

Putting through triage

	Preeti Raghunath(:Preeti)
	Comment 42 • 10 years ago

Comment on attachment 8393959 [details]
PR to review@master

will review in triage

	Preeti Raghunath(:Preeti)
	Comment 43 • 10 years ago

Comment on attachment 8393959 [details]
PR to review@master

taking in 1.4

	Ryan VanderMeulen [:RyanVM]
	Comment 44 • 10 years ago

v1.4: https://github.com/mozilla-b2g/gaia/commit/c6cf82cb56681dbb9ef48cb41fdcd6380da01b26