832842 - [Dialer] Keypad.js didn't consider mozAudioWrite() as non-blocking function.

Status: RESOLVED FIXED

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

Description

[Marco Chen [:mchen]]

STR:
1. Launch the dialer app.
2. Pressing keys very quickly and many times until numbers are greater then show area.

Expected Result:
Key tones and numbers update are synchronization with each action of key pressing.

Actual Result:
Key tones and numbers update are continue to fire after finishing the actions of key pressing.

Comment 1

[Marco Chen [:mchen]]

Provide some points here after doing some investigation.

1. Each key tone will spend about 80ms and it is fired by mozWriteAudio() in main thread.
   (Could we play this sound off main thread? (ex: playing by file - decoding thread))

2. The _updatePhoneNumberView() in keypad.js is called in the action of mouse up. Is this a necessary one?

3. The formatPhoneNumber() in keypad.js also spent about 40ms when numbers are greater then view area. ()

Comment 2

[Marco Chen [:mchen]]

Hi etinenne,

Could you give some feedback on this case or suggest anyone here?
Very thanks for your help.

Comment 3

[Etienne Segonzac (:etienne)]

(In reply to Marco Chen [:mchen] from comment #2)
> Hi etinenne,
> 
> Could you give some feedback on this case or suggest anyone here?
> Very thanks for your help.

Playing a file won't cut it because we need to support long-press (and long play of the tone).
(This feature is only activated while you're on a call).

But IIRC David had some ideas to improve the performance of the tone generation, might be the lowest hanging fruit here.

Comment 4

[Marco Chen [:mchen]]

(In reply to Etienne Segonzac (:etienne) from comment #3)
> (In reply to Marco Chen [:mchen] from comment #2)
> Playing a file won't cut it because we need to support long-press (and long
> play of the tone).
> (This feature is only activated while you're on a call).
> 
> But IIRC David had some ideas to improve the performance of the tone
> generation, might be the lowest hanging fruit here.

Hi Etienne,

Thanks for your reply here.

And I tried to disable the keypad sound from setting then the performance of keypad in dialer app will be very good. So I think the 80ms from key tone is a bottleneck in this bug.

How about my question 2 on comment 1? Is that necessary or can be removed?

Hi David,

Could you share your idea on reducing time consumption on tone generation?
Thanks.

Comment 5

[Matthew N. [:MattN]]

(In reply to Marco Chen [:mchen] from comment #4)
> Could you share your idea on reducing time consumption on tone generation?

Bug 820104 is probably part of it.

Comment 6

[Marco Chen [:mchen]]

(In reply to Matthew N. [:MattN] from comment #5)
> (In reply to Marco Chen [:mchen] from comment #4)
> > Could you share your idea on reducing time consumption on tone generation?
> 
> Bug 820104 is probably part of it.

After doing more check, the generation of tone frames just spent about 10ms only. Please refer to the sample log as below.

01-22 08:23:37.400 I/GeckoDump( 1020): Generation Frames
01-22 08:23:37.410 I/GeckoDump( 1020): mozWriteAudio
01-22 08:23:37.490 I/GeckoDump( 1020): mozWriteAudio End.

So I think the bottleneck here is on mozWriteAudio not Generation Frames.

Is it possible that we play a tone via file in off-call state but play via mozWriteAudio() in in-call state?

Comment 7

[Etienne Segonzac (:etienne)]

> Is it possible that we play a tone via file in off-call state but play via
> mozWriteAudio() in in-call state?

Another issue with playing from a file was the latency...

Comment 8

[Etienne Segonzac (:etienne)]

> How about my question 2 on comment 1? Is that necessary or can be removed?

I'd say necessary but probably optimizable.

Comment 9

[David Flanagan [:djf]]

(In reply to Matthew N. [:MattN] from comment #5)
> (In reply to Marco Chen [:mchen] from comment #4)
> > Could you share your idea on reducing time consumption on tone generation?
> 
> Bug 820104 is probably part of it.

Yes. I think those basic waveforms could be pre-computed. We shouln't have to do any floating-point arithmetic. Coment 6 implies that there are other problems.

I'm not sure I understand this bug correctly. In the initial description it sounds as if the issue is synchronization between display update and tones. When I try to reproduce, the tones seem pretty good to me.  But I can see that the display does not always update promptly.  So are the tones involved at all here?

Here are my (mostly uninformed) thoughts: 

When we are on call, we start the done on touchstart and end the tone on touchend.  But when we're not on a call yet, we play a tone of fixed duration. It is always the same no matter how long or short the tap is.  We differ from android on this.

So if the user taps really quickly, then we're still busy playing a tone while we're also calling _updatePhoneNumberView().  (I don't know what that does or why we need to do it on mouse up.)  Maybe if we stopped the tone when the user released we'd see an improvement?

The description seems to imply that the bug gets worse as the number gets longer.  If so, we should check to see if the font-size switching is slowing us down.  

And we should check whether formatPhoneNumber() is O(n*n) or something. Is there an inefficient algorithm that gets slower and slower as the phone number gets longer?

Comment 10

[Marco Chen [:mchen]]

(In reply to Etienne Segonzac (:etienne) from comment #7)
> > Is it possible that we play a tone via file in off-call state but play via
> > mozWriteAudio() in in-call state?
> 
> Another issue with playing from a file was the latency...

May I know the latency you mentioned here? And is it played by a wave file?
Or if you can provide the code I can test it.

Very thanks.

Comment 11

[Marco Chen [:mchen]]

(In reply to Etienne Segonzac (:etienne) from comment #8)
> > How about my question 2 on comment 1? Is that necessary or can be removed?
> 
> I'd say necessary but probably optimizable.

In the code section of mouseup, I didn't see anything changed PhoneNumberView which caused we need to call this._updatePhoneNumberView(). Isn't it?

Comment 12

[Marco Chen [:mchen]]

(In reply to David Flanagan [:djf] from comment #9)
> I'm not sure I understand this bug correctly. In the initial description it
> sounds as if the issue is synchronization between display update and tones.
> When I try to reproduce, the tones seem pretty good to me.  But I can see
> that the display does not always update promptly.  So are the tones involved
> at all here?

Oh, sorry to the unclear description. 
The abnormal symptom of this bug is that 
  1. Pressing keys as much and quick as possible.
  2. After finishing the key press, you still hear and see the update of PhoneNumberView.
     (Issue here, the update of PhoneNumberView doesn't sync with your each key pressing)
 
>So if the user taps really quickly, then we're still busy playing a tone while we're also >calling _updatePhoneNumberView().  (I don't know what that does or why we need to do it on >mouse up.)  Maybe if we stopped the tone when the user released we'd see an improvement?

Yes about _updatePhoneNumberView() in mouseup, I also raised the question on Comment 11.
For playing a tone it is a blocking function there. So we need to wait this function done then we just can process mouseup event.

> The description seems to imply that the bug gets worse as the number gets
> longer.  If so, we should check to see if the font-size switching is slowing
> us down.  
> 
> And we should check whether formatPhoneNumber() is O(n*n) or something. Is
> there an inefficient algorithm that gets slower and slower as the phone
> number gets longer?

Yes, the each of formatPhoneNumber() on mouseup/mousedown will spend about 40ms too.

Comment 13

[Etienne Segonzac (:etienne)]

(In reply to Marco Chen [:mchen] from comment #11)
> (In reply to Etienne Segonzac (:etienne) from comment #8)
> > > How about my question 2 on comment 1? Is that necessary or can be removed?
> > 
> > I'd say necessary but probably optimizable.
> 
> In the code section of mouseup, I didn't see anything changed
> PhoneNumberView which caused we need to call this._updatePhoneNumberView().
> Isn't it?

No, but we're not (calling _updatePhoneNumberView() on mouseup).

Comment 14

[Etienne Segonzac (:etienne)]

CC'ing German who probably knows the non-tone part of keyboard.js better than me.

Comment 15

[Etienne Segonzac (:etienne)]

(In reply to Marco Chen [:mchen] from comment #10)
> May I know the latency you mentioned here? And is it played by a wave file?
> Or if you can provide the code I can test it.
> 
> Very thanks.

Don't have a patch handy for this, sorry.

Comment 16

[Germán Toro del Valle (:gtorodelvalle)]

Yeah, I kind of got lost in your comments, sorry :( As Etienne points out we are not calling _updatePhoneNumberView() on mouseup. So I would appreciate if you could rephrase the "non-tone part of keyboard.js" question I may be able to shed some light on ;-) Thanks!

Comment 17

[Marco Chen [:mchen]]

(In reply to gtorodelvalle from comment #16)
> Yeah, I kind of got lost in your comments, sorry :( As Etienne points out we
> are not calling _updatePhoneNumberView() on mouseup. So I would appreciate
> if you could rephrase the "non-tone part of keyboard.js" question I may be
> able to shed some light on ;-) Thanks!

Hi Etienne and German,

Thanks for your clarification.
I just noted that it was removed on Bug 827753 (two weeks ago).

So now there are two points to do improvement.
  1. about 80ms from key tone during mouse down event.
  2. 40ms from updatePhoneNumberView() during mouse down event.

German, may I know any idea on point 2?
Etienne, I will try to measure the time from playing file.

Comment 18

[Germán Toro del Valle (:gtorodelvalle)]

Hi guys, I did some research on the matter and I would say we definitely do have space for improvement. Right now we are doing really weird stuff based on the current view and some hidden faked view ;-999 In fact, I would base the new solution on my tests at http://jsbin.com/ageyos/12/edit just in case you may want to comment something. To test it, just write characters in the #textElement element on the left hand side of the screen :-) The duration of the algorithm is printed to console and as you'll see it is pretty low and, probably more important, stable ;-) , which is kind of obvious if you check the algorithm :-p

The algorithm is even improvable taking into consideration that we do have maximum and minimum font sizes and consequently if we are currently using the maximum font size and text is added, we can directly return this maximum font size. The opposite regarding the minimum font size.

On the other hand, there is also place for improvement regarding the addEllipsis function. This function and the one calculating the new font size it taking most of the time of the processing.

Last for not least, should we nominate this bug? Just to prioritize my work :)

Thanks!

Comment 19

[Marco Chen [:mchen]]

(In reply to gtorodelvalle from comment #18)
Hi German,

I think this one will be not a blocking issue. But if it is a simple way to do improvement and no risk then we can try to ask the flag. This is my thought, maybe you can still raise the flag.

Comment 20

[Marco Chen [:mchen]]

Hi Etienne,

For 80ms from MozWriteAudio(), I think if we can move this blocking function to another thread then it can improve this bug very well. 

Method 1: I tried to modify mozWriteAudio() to post a run event for playing audio in another thread and the keys pressing will be became smooth.

Method 2: Do you know any way to do this (play audio in another thread) in Gaia side?

Thanks.

Comment 21

[Paul Adenot (:padenot)]

Marco,

mozWriteAudio, specifically on B2G, use the cubeb audio backend, that make AudioStream::Write non-blocking most of the times, granted you have room the the cubeb buffer (the size of the buffer is exactly one second of audio data), see [1]. This is no true on fennec. But last time I checked, mozWriteAudio was busted on fennec.

Then the BufferedAudioStream::DataCallback method is called, see [2]. This is called periodically by the cubeb library, to pass the audio data to the OpenSL backend. This is done on a separate thread (a thread that is used only to play audio).

Don't hesitate to ask more questions.

[1]: http://mxr.mozilla.org/mozilla-central/source/content/media/AudioStream.cpp#793
[2]: http://mxr.mozilla.org/mozilla-central/source/content/media/AudioStream.cpp#1050

Comment 22

[Marco Chen [:mchen]]

(In reply to Paul Adenot (:padenot) from comment #21)
Hi Paul, Thanks for your reply here.

I inserted log into Sydney_audio_gonk.cpp and the log is printed. So the audio library used on B2G is Sydney (NativeAudioStream) not cubed (BufferedAudioStream). I also checked that even MOZ_CUBED is enabled in default but GetUseCubeb() returns 0 on current base.

By the way the current B2G doesn't use openSL backend but the audio flinger from Android.

Comment 23

[Paul Adenot (:padenot)]

Interesting, maybe we could experiment using the OpenSL backend.

Comment 24

[Marco Chen [:mchen]]

Attachment: Part 1: Gecko Patch v1

Hi Robert,

The purpose of this patch is try to reduce the blocking time of Gaia app calling MozWriteAudio(...) so we can improve the speed of key responding.

The idea is that since MozWriteAudio() will create a new array for writing data then we can split the audiostream.write() to another thread. Maybe we can keep MozWriteAudio() and create a new function for this synchronization method.

Thanks for your feedback in advance.

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 707006 [details] [diff] [review]
Part 1: Gecko Patch v1

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

I think the runnable should be a separate object from the nsHTMLAudioElement.
 
You should document that you're working around Write() being a blocking call.

We should probably take this only on the B2G branch. In the next version of B2G I expect we'll have a version of Web Audio that you can use for this which will be more efficient, and simpler for app authors.

::: content/html/content/public/nsHTMLAudioElement.h
@@ +57,5 @@
>    virtual nsIDOMNode* AsDOMNode() { return this; }
> +
> +private:
> +  nsAutoArrayPtr<mozilla::AudioDataValue> mAudioData;
> +  mozilla::ReentrantMonitor mReentrantMonitor;

Document what fields this lock protects.

::: content/html/content/src/nsHTMLAudioElement.cpp
@@ +217,5 @@
>    }
>  
> +  {
> +    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
> +    int64_t position = mAudioStream->GetPositionInFrames();

Why do we need this lock here?

Comment 26

[Marco Chen [:mchen]]

Attachment: Part 1: Gecko Patch v2

Thanks for the suggestion and follow the comments.

>::: content/html/content/src/nsHTMLAudioElement.cpp
>@@ +217,5 @@
>>    }
>>  
>> +  {
>> +    ReentrantMonitorAutoEnter mon(mReentrantMonitor);
>> +    int64_t position = mAudioStream->GetPositionInFrames();

> Why do we need this lock here?

Since this is a read function only, I removed the lock here.

Comment 27

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 707476 [details] [diff] [review]
Part 1: Gecko Patch v2

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

::: content/html/content/public/nsHTMLAudioElement.h
@@ +11,5 @@
>  #include "nsHTMLMediaElement.h"
> +#ifdef MOZ_B2G
> +#include "AudioSampleFormat.h"
> +#include "mozilla/ReentrantMonitor.h"
> +#endif

Remove all the MOZ_B2G ifdefs from this patch

@@ +66,5 @@
> +  nsAutoArrayPtr<mozilla::AudioDataValue> mAudioData;
> +  // The calling sequence as below should be synchronized for protecting
> +  // audio data & stream.
> +  // MozSetup() -> MozWriteAudio() -> mAudioStream->Write()
> +  mozilla::ReentrantMonitor mReentrantMonitor;

What is this protecting and why is it needed?

I think you could just have a separate nsRunnable subclass that doesn't use a lock and just writes some data to the output stream and returns.

::: content/html/content/src/nsHTMLAudioElement.cpp
@@ +202,5 @@
> +    rv = NS_NewNamedThread("mozAudio Write",
> +                           getter_AddRefs(mAudioThread),
> +                           nullptr,
> +                           MEDIA_THREAD_STACK_SIZE);
> +    NS_ABORT_IF_FALSE(NS_SUCCEEDED(rv), "Can't create mozAudio writing thread");

Creating a thread for every write seems excessive and I'm surprised it doesn't cause more performance problems than it solves!

I think we need to create a thread and keep it around for a while. Probably the lifetime of the audio element, since that's simple. So keep a pointer to it from the element.

Always dispatching write-audio runnables to the same thread ensures that the writes won't get out of order, too.

Comment 28

[Marco Chen [:mchen]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #27)
> Comment on attachment 707476 [details] [diff] [review]
> Part 1: Gecko Patch v2
> 
> Review of attachment 707476 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Remove all the MOZ_B2G ifdefs from this patch
> 

I misunderstand the "B2G Branch only" on comment 25. Will remove them.

> What is this protecting and why is it needed?
>
> I think you could just have a separate nsRunnable subclass that doesn't use a lock and just > writes some data to the output stream and returns.

Will follow in next patch.


> I think we need to create a thread and keep it around for a while. Probably
> the lifetime of the audio element, since that's simple. So keep a pointer to
> it from the element.
> 
> Always dispatching write-audio runnables to the same thread ensures that the
> writes won't get out of order, too.

 if (mAudioThread == nullptr)
 {
    rv = NS_NewNamedThread("mozAudio Write",

 Before creating thread, I will check whether mAudioThread is null or not. So it should be created once per audio tag.

Thanks.

Comment 29

[Marco Chen [:mchen]]

Attachment: Part 1: Gecko Patch v3

1. Removing B2G definition.
2. Add a new subclass called WriteAudioRunnable in nsHTMLAudioElement.
3. Introduce a new ReentrantMonitor for protecting AudioStream between MozSetup() * WriteAudioRunnable::Run().
4. Introduce a new thread for WriteAudioRunnable and it will be created once only when first time of calling MozWriteAudio().

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Your patch gets the nsHTMLAudioElement's mAudioStream off the main thread. We shouldn't touch DOM elements off the main thread in any way. How about giving the runnable a reference to the mAudioStream directly. Then I think you don't need this lock at all.

Comment 31

[Marco Chen [:mchen]]

Hi Robert & Matthew,

Consider to the case as below, I think I need to change AudioStream from nsAutoPtr to nsRefPtr in nsHTMLMediaElement so the AudioStream will inherit nsSupports as an nsAudioStream again.

Case:
  If web App calls mozWriteAudio(...) many times then call mozSetup() directly, there will be many runnable events queued in the new thread. In order to let these queued events can still use AudioStream, AudioStream should be protected by ref count.

Or in nsHTMLAudioElement, I can write a wrapper class contained AudioStream which inherits nsSupports, so it can be used by nsRefPtr.

Could you give me the suggestion on these two ideas? Many thanks.

Comment 32

[Marco Chen [:mchen]]

As the suggestion from Padenot,

The third option is to use RefPtr class to wrap AudioStream in nsHTMLMediaElement.

Comment 33

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Marco Chen [:mchen] from comment #31)
> Or in nsHTMLAudioElement, I can write a wrapper class contained AudioStream
> which inherits nsSupports, so it can be used by nsRefPtr.

Let's do this one.

Comment 34

[Marco Chen [:mchen]]

Comment on attachment 708013 [details] [diff] [review]
Part 1: Gecko Patch v3

To update the status here.

1. Thanks for Matthew's confirmation. I found that the root cause on 80ms spent for each key tone is caused by wrong value from AudioStream::Available().

2. AudioTrack provided a mechanism based on callback function. So we can be notified about what exact timing AudioTrack can eat how much data.

3. I tried to use pthread mutex between sydney::write() and AudioTrack::callback() for synchronizing them. And this will get accurate available value and not block mozWriteAudio().

4. To evaluate the effort on libcubed with AudioTrack::callback on B2G or use point 3 above. Then Gaia side need to take care the wrote length returned from mozWriteAudio().

Comment 35

[Marco Chen [:mchen]]

Hi Etienne,

Since the frame size now on unagi is 435 samples, could you split these 1200 (150ms) now to three pieces for calling mozWriteAudio with non-blocking call? 
Maybe use setInterval() to schedule these 3 samples every 45ms. This is very short term solution on this stage but with lower effort and risk.

Hi German,

addEllipsis() in keypad.js is still a bottleneck here. So if you have time please help to provide any your great idea? 

Very thanks.

Comment 36

[David Flanagan [:djf]]

I'm guessing that Marco actually meant to set needinfo for German instead of me. So setting need info for him.

Comment 37

[Germán Toro del Valle (:gtorodelvalle)]

(In reply to David Flanagan [:djf] from comment #36)
> I'm guessing that Marco actually meant to set needinfo for German instead of
> me. So setting need info for him.

Thanks to David for noticing me ;-)

Regarding the addEllipsis() function, since this function is used once the font size is determined, I would suggest accomplishing it via CSS adding the ellipsis to the appropriate side using the "direction" property as it is done here: http://jsfiddle.net/gtorodelvalle/EFZ3r/2/ Adding a simple class to the appropriate element would do the trick ;-)

Comment 38

[Etienne Segonzac (:etienne)]

Attachment: Gaia patch (dialer mozWriteAudio call changes)

Something like that? (couldn't really tell the difference).

Comment 39

[Matthew Gregan [:kinetik]]

(In reply to Marco Chen [:mchen] Away from 2/9 ~ 2/17 from comment #34)
> 1. Thanks for Matthew's confirmation. I found that the root cause on 80ms
> spent for each key tone is caused by wrong value from
> AudioStream::Available().

I have a fix for this.  I've attached it to bug 833724.  Hopefully that'll solve the original problem and we won't need to use a new thread to call mozWriteAudio now.

I did discover that Gaia is misusing the mozWriteAudio API by not checking how much was actually written.  This never showed up before because we'd always try to write the entire buffer and block, if the buffer was full.  With AudioStream::Available() fixed on Gonk, we now write enough to fill the buffer and mozWriteAudio can return a value less than the size of the buffer it was passed.

Comment 40

[Marco Chen [:mchen]]

Comment on attachment 712471 [details] [diff] [review]
Gaia patch (dialer mozWriteAudio call changes)

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

Hi Etienne,

Thanks for your follow up first.

::: apps/communications/dialer/js/keypad.js
@@ +98,5 @@
> +    for (var i = 400; i < initialSoundData.length; i += 400) {
> +      setTimeout(function next(i) {
> +        audio.mozWriteAudio(initialSoundData.subarray(i, i + 400));
> +      }, 45, i);
> +    }

As Matthew said on comment 39, the mozWriteAudio(...) will be a non-blocking function and the return value means how many bytes wrote to audio buffer already. So maybe you can check the return value to decide whether we need to call setTimeout for filling next chunk of audio data. 
ex: total 1200 samples.
    first run writing 435 samples. (call setTimeout)
    second run writing 435 samples. (call setTimeout)
    third run writing 330 samples. (no need to call setTimeout)

And stop the existing timer when key is releasing.

Comment 41

[Marco Chen [:mchen]]

Comment on attachment 712471 [details] [diff] [review]
Gaia patch (dialer mozWriteAudio call changes)

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

Hi Etienne,

change the flag to notice you that I already reply you on comment 40.
Thanks.

::: apps/communications/dialer/js/keypad.js
@@ +98,5 @@
> +    for (var i = 400; i < initialSoundData.length; i += 400) {
> +      setTimeout(function next(i) {
> +        audio.mozWriteAudio(initialSoundData.subarray(i, i + 400));
> +      }, 45, i);
> +    }

As Matthew said on comment 39, the mozWriteAudio(...) will be a non-blocking function and the return value means how many bytes wrote to audio buffer already. So maybe you can check the return value to decide whether we need to call setTimeout for filling next chunk of audio data. 
ex: total 1200 samples.
    first run writing 435 samples. (call setTimeout)
    second run writing 435 samples. (call setTimeout)
    third run writing 330 samples. (no need to call setTimeout)

And stop the existing timer when key is releasing.

Comment 42

[Marco Chen [:mchen]]

Attachment: Gaia Patch v1

Hi Etienne,

Recap that mozWriteAudio() is a non-blocking function so it will just write a limited amount of data into audio backend. The patch here is tried to check the return length from mozWriteAudio() then use setInterval to

  1. In non-calling state, just continue to write until all data are done (1200 frames)
  2. In calling state, continue to write until .stop is called.

Thanks for your review in advance.

Comment 43

[Etienne Segonzac (:etienne)]

Comment on attachment 727555 [details] [diff] [review]
Gaia Patch v1

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

The code looks fine (minus the the liniting issues ;)) but it causes an under-run while doing long presses.
To be clear, with a moz-central gecko we get under-run issues with or without the patch, but on b2g18 the patch triggers the issue.

Simply lowering the setInterval doesn't seem to cut it.

Comment 44

[Marco Chen [:mchen]]

Hi Etienne,

1. I tried to patch in m-c but I didn't be aware of under-run issue.
   Could you give the symptom of this under-run issue.

2. On m-c codes, mozAudioWrite() in gonk platform is fixed to be non-blocking call.
   And in unagi device, the frameCount is set to 435 (about 54ms).  So on the case  
   of long press, it's interval is 60ms (plus the 10ms deviation then worst case 
   will be 70ms). I can see the under-run issue.

3. In the plan now, bug 840311 introduced frameCount to double (870 in unagi and about 108ms). The patch here used interval as 30ms (worst to 40ms). So I think there should not be under-run the buffer.

Will test again too.

Comment 45

[Marco Chen [:mchen]]

By the way, sydney backend is removed on m-c and instead by cubeb backend. So may I know you test is on sydney or cubeb? thanks.

Comment 46

[Marco Chen [:mchen]]

Attachment: Gaia patch v2

Hi etienne,

I found that is not a under-run issue and that is caused by my patch doesn't re-generate key tone with sustaining wave only.
Could you help to review this one?

Thanks.

Comment 47

[Etienne Segonzac (:etienne)]

Comment on attachment 728038 [details] [diff] [review]
Gaia patch v2

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

r=me with the lint errors fixed
Can you also remove the |this._audio.volume = 1;| line? (we initially setup _audio.volume to 0.5, and it's fine)

This is working well :)

::: apps/communications/dialer/js/keypad.js
@@ +110,5 @@
>      this._intervalID = setInterval((function audioLoop() {
> +      start = start + wrote;
> +      // Continuing playing until .stop() is called for long press in calling
> +      // state. Or just play one round of data in non calling state.
> +      if (this._stopping || ( start == 1200 && shortPress == true)) {

lint error: |(start == 1200... with no space

@@ +130,2 @@
>        if (this._audio != null)
> +        wrote = this._audio.mozWriteAudio(initialSoundData.subarray(start, 1200));

lint error: line too long

Comment 48

[Jason Duell]

should this bug be marked dependent on bug 833724 and bug 840311?  See bug 840311 comment 17

Comment 49

[Marco Chen [:mchen]]

Issue:
  mozAudioWrite() is non-blocking function and keypad.js used it to output key tone. But it didn't handle the return value when writing length is bigger then audio backend buffer. In that case mozAudioWrite will return what it wrote already immidately. If we don't handle it then the key tone will miss some frames.

Test:
  Tested it on the case of non in-call and in-call state.

Risk:
  none.

Comment 50

[Lukas Blakk [:lsblakk] use ?needinfo]

This doesn't block a blocker and we would most likely ship with this issue since it show up under slightly abnormal use conditions so we'll track instead and please go ahead with an uplift nomination for the patch.

Comment 51

[Marco Chen [:mchen]]

Attachment: Gaia patch v3

1. To fix the nits reported from review comment.
2. Add a constant variable called kKeyToneFrames to represent 1200 in current code.

Comment 52

[Marco Chen [:mchen]]

About add a constant variable, I will ask for the comment from reviewer before checking in the code.

Comment 53

[Etienne Segonzac (:etienne)]

Comment on attachment 731744 [details] [diff] [review]
Gaia patch v3

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

sounds good (literally), with the last lint issue fixed :)

::: apps/communications/dialer/js/keypad.js
@@ +130,3 @@
>        if (this._audio != null)
> +        wrote = this._audio.mozWriteAudio(
> +          initialSoundData.subarray(start,kKeyToneFrames));

small lint issue here, space missing after the |,|

Comment 54

[Marco Chen [:mchen]]

Attachment: gaia patch v4

Thanks for the review and fix that nit.

Comment 55

[Etienne Segonzac (:etienne)]

Cool, do you need a checkin?

Comment 56

[Marco Chen [:mchen]]

I do the pull request already.

Comment 57

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/15519485961602e85df58aa5cfd850555559965b