Closed Bug 972308 Opened 11 years ago Closed 11 years ago

[DIALER]slow response when click on dialer panel

Categories

(Firefox OS Graveyard :: Gaia::Dialer, defect, P2)

ARM
Gonk (Firefox OS)
defect

Tracking

(blocking-b2g:1.3T+, b2g18 unaffected, b2g-v1.1hd unaffected, b2g-v1.2 unaffected, b2g-v1.3 wontfix, b2g-v1.3T verified, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
1.4 S4 (28mar)
blocking-b2g 1.3T+
Tracking Status
b2g18 --- unaffected
b2g-v1.1hd --- unaffected
b2g-v1.2 --- unaffected
b2g-v1.3 --- wontfix
b2g-v1.3T --- verified
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: ying.xu, Assigned: ying.xu)

References

Details

(Keywords: perf, Whiteboard: [c=progress p= s= u=])

Attachments

(3 files, 2 obsolete files)

(deleted), image/png
Details
(deleted), text/x-github-pull-request
gsvelto
: review+
etienne
: feedback+
Details
(deleted), text/x-github-pull-request
gsvelto
: review+
Details
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
blocking-b2g: --- → 1.3T?
Summary: [DIALER]slow response when click on dialer panel → [tarako][DIALER]slow response when click on dialer panel
Target Milestone: --- → 1.4 S1 (14feb)
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
Triage to 1.3T+
blocking-b2g: 1.3T? → 1.3T+
Depends on: 917193
Was this tested before or after bug 917193 landed? That bug should have improved the situation here IMHO.
(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.
And I noticed that deleting a figure by clicking on the 'X' button is very quick.
Keywords: perf
Priority: -- → P1
Whiteboard: [c= p= s= u=tarako]
Whiteboard: [c= p= s= u=tarako] → [c=progress p= s= u=tarako]
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.
Whiteboard: [c=progress p= s= u=tarako] → [c=progress p= s= u=tarako] [SI testing blocker]
Whiteboard: [c=progress p= s= u=tarako] [SI testing blocker] → [c=progress p= s= u=tarako] [SI-testing-blocker]
Depends on: 967440
Component: Gaia → Gaia::Dialer
QAWANTED to confirm latest status. thanks
Flags: needinfo?(brhuang)
Keywords: qawanted
William, please help on this confirmation. If the keyboard response is acceptable, upload a video for partner.
Flags: needinfo?(brhuang) → needinfo?(whsu)
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.
Flags: needinfo?(whsu)
Attached image Dialer_keypad_20140310.png (deleted) —
James, seem like we can resolve this bug as workforme, can you confirm on your side? thanks
Flags: needinfo?(james.zhang)
(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.
Flags: needinfo?(james.zhang)
Keywords: qawanted
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.
Flags: needinfo?(james.zhang)
(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?
Flags: needinfo?(james.zhang) → needinfo?(ying.xu)
back to 1.3T? as we are waiting further information
blocking-b2g: 1.3T+ → 1.3T?
triage: minus for now. please nominate again if this bug still happens
blocking-b2g: 1.3T? → -
Whiteboard: [c=progress p= s= u=tarako] [SI-testing-blocker] → [c=progress p= s= u=tarako]
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
Flags: needinfo?(ying.xu)
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; } }
Summary: [tarako][DIALER]slow response when click on dialer panel → [DIALER]slow response when click on dialer panel
Ying, did you want to submit an official patch for review?
Severity: critical → normal
blocking-b2g: - → backlog
Flags: needinfo?(ying.xu)
Priority: P1 → P2
Whiteboard: [c=progress p= s= u=tarako] → [c=progress p= s= u=]
Target Milestone: 1.4 S1 (14feb) → ---
Attached file PR to review , based on v1.3t (obsolete) (deleted) —
Attachment #8392000 - Flags: review?(etienne)
Flags: needinfo?(ying.xu)
The audio is still missing sometimes. We know some problems in our audio drivers.We will fix that.
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.
Attachment #8392000 - Flags: review?(gsvelto)
Attachment #8392000 - Flags: review?(etienne)
Attachment #8392000 - Flags: feedback+
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.
Flags: needinfo?(etienne)
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 :)
Flags: needinfo?(etienne)
(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 :-|
Attached file PR to review @master (deleted) —
Attachment #8392640 - Flags: review?(etienne)
Attached file PR to review @v1.3t (obsolete) (deleted) —
Attachment #8392000 - Attachment is obsolete: true
Attachment #8392000 - Flags: review?(gsvelto)
Attachment #8392641 - Flags: review?(etienne)
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...).
Attachment #8392640 - Flags: review?(gsvelto)
Attachment #8392640 - Flags: review?(etienne)
Attachment #8392640 - Flags: feedback+
Attachment #8392641 - Flags: review?(etienne) → review?(gsvelto)
Comment on attachment 8392640 [details] PR to review @master r=me with the nits addressed, see my comment on GitHub.
Attachment #8392640 - Flags: review?(gsvelto) → review+
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.
Attachment #8392641 - Attachment is obsolete: true
Attachment #8392641 - Flags: review?(gsvelto)
Assignee: nobody → ying.xu
Status: NEW → ASSIGNED
Attached file PR to review@master (deleted) —
update according to Comment 29 Gabriele Svelto [:gsvelto] 2014-03-19 08:39:27 PDT
Attachment #8393959 - Flags: review?(gsvelto)
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.
Attachment #8393959 - Flags: review?(gsvelto) → review+
Merged to gaia/master 75cc0092a71d4a4e8ee2c81200ed60cf810f05e2 Nom'ing again as this might be needed for decent keypad tones on Tarako.
Status: ASSIGNED → RESOLVED
blocking-b2g: backlog → 1.3T?
Closed: 11 years ago
Resolution: --- → FIXED
hi YingXu, how much improvement do you see here? lets understand the improvement here before we decide for tarako. thanks
Flags: needinfo?(ying.xu)
(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
Flags: needinfo?(ying.xu)
triage: 1.3T+ to improve performance. can you please update it and resolved fixed on status-b2g-v1.3T? thanks
blocking-b2g: 1.3T? → 1.3T+
Why has this patch not been uplifted yet?
Target Milestone: --- → 1.4 S4 (28mar)
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
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
Attachment #8393959 - Flags: approval-gaia-v1.4?
Putting through triage
blocking-b2g: 1.3T+ → 1.4?
Comment on attachment 8393959 [details] PR to review@master will review in triage
Attachment #8393959 - Flags: approval-gaia-v1.4?
Comment on attachment 8393959 [details] PR to review@master taking in 1.4
Attachment #8393959 - Flags: approval-gaia-v1.4+
blocking-b2g: 1.4? → 1.3T+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: