924454 - Improve busy tone implementation

Status: RESOLVED FIXED

Description

[Beatriz Rodríguez [:brg]]

This issue had been reported during certification of version 1.1. We got a waiver from carrier for v1.1 but we need to improve it in future OS releases.

STR: Tested with Unagi, master, Moz RIL, Gecko-1e4f6fc.Gaia-1499c58: 
- Call a user busy (without having the call waiting or voicemail activated)
- Try to hear the busy tone in your unagi device.

You will hear a strange busy tone that need to be improved in two different ways:
- Intermittent effect: the tone is not cleanly played.
- Length of the tone: it is played for a very short time. Please increase it some seconds.

Comment 1

[Preeti Raghunath(:Preeti)]

koi+ based on cert waiver for 1.1

Comment 2

[Joe Cheng [:jcheng] (please needinfo)]

ni? Gabriele. to verify if this is a dup

Comment 3

[Alexandre LISSY :gerard-majax]

Isn't somehow related to bug 910267 ?

When working on this one, I just re-ued the ANSI busy tone that was previously implemented. It should be a 3-seconds tone. And as far as I remember testing it, it was okay.

Can you document more closely what you mean by « the tone is not cleanly played » and the short time ?

Comment 4

[Gabriele Svelto [:gsvelto]]

I think this is a dupe of bug 922569, the second part of the bug certainly is (too short DTMF tone). Since I'm working on a fix there I'll close this and if we still have the same problem when bug 922569 lands then we can reopen this one.

Comment 5

[Beatriz Rodríguez [:brg]]

(In reply to Gabriele Svelto [:gsvelto] from comment #4)
> I think this is a dupe of bug 922569, the second part of the bug certainly
> is (too short DTMF tone). Since I'm working on a fix there I'll close this
> and if we still have the same problem when bug 922569 lands then we can
> reopen this one.
> 
> *** This bug has been marked as a duplicate of bug 922569 ***
This bug is about busy tone, not DTMF tones, sorry. Reopening it.

Comment 6

[Beatriz Rodríguez [:brg]]

(In reply to Alexandre LISSY :gerard-majax (off 16/09-14/10) from comment #3)
> Isn't somehow related to bug 910267 ?
> 
Before that bug was implemented, there was no busy tone at all. This bug is because the quality of the tone is quite bad :-(
> When working on this one, I just re-ued the ANSI busy tone that was
> previously implemented. It should be a 3-seconds tone. And as far as I
> remember testing it, it was okay.
> 
> Can you document more closely what you mean by « the tone is not cleanly
> played » and the short time ?
Sorry, let me try to be more descriptive:
"The tone is not cleanly played" --> the tone is played intermittentely, it is faltering, not a clean busy tone.
"The short time" --> the tone is stopped too soon besides, if you cannot hear it well because of the faltering issue, the length(3 sec) is not enough. Can we make it double length around 6 sec?
The expected busy tone should look like this: http://www.soundsnap.com/node/47152

I hope I had been more clear now, please contact me if something is not clear enough.

Comment 7

[Jason Smith [:jsmith]]

Renoming for more discussion - this might need to be punted to 1.3 at this point.

Comment 8

[Beatriz Rodríguez [:brg]]

(In reply to Jason Smith [:jsmith] from comment #7)
> Renoming for more discussion - this might need to be punted to 1.3 at this
> point.
Please read the description of this bug and comment #1. This is a blocker, set back the KOI+. We will not pass cert process with this bug again.

Comment 9

[Preeti Raghunath(:Preeti)]

cert blocker hence koi+

Comment 10

[Gabriele Svelto [:gsvelto]]

I've had some time to analyze this bug and the fix is probably going to be non-trivial. I've tried adjusting the length of the tone to 6 seconds but while it's now clearly audible it's still quirky and jittery. Long story short, the TonePlayer object used to synthesize the sound uses timers which seem not to be firing at the right time. I'll have to debug it and figure what's wrong with it before we can get a clean tone.

Alternatively we might consider using a pre-recorded tone.

Comment 11

[David Palomino [:dpalomino]]

Hi Gabriele, 

I think it'd be preferrable to use the standard way of reproducing this tone (the TonePlayer object), as this would assure that we're on the proper track if something is changed in the future and avoid potential regressions... 

Anyway, if should choose between implement the pre-recorded tone, or to delay this to 1.3, I think it'd be better to have something ready for koi. 

Thanks!!
David

Comment 12

[David Palomino [:dpalomino]]

Hi, 

After a talk with Gabriele (thanks a lot Gabriele), he confirmed he was taking care of this. 

Thanks!
David

Comment 13

[Gabriele Svelto [:gsvelto]]

Created attachment 832948 [details] [diff] [review]
[PATCH] Make the busy tone longer and clearer

I've lengthened the busy tone to 6 seconds but while doing so I've noticed that only the first "beep" sounded jittery while the following ones tended to be clear. I couldn't immediately put my finger on why this happened but I eventually attributed it to high CPU usage right at the beginning of the busy tone; this is normal behavior as we're showing a panel at the same time we start playing the tone. Let me explain why this is causing the glitch:

The tone is composed of two distinct frequencies which we instruct to start at the same time:

http://is.gd/XETinV

However since that time is somewhere in the past when we reach that point (we got it from |currentTime| a few lines above) the tones will actually start whenever we connect them to a node that is already playing. 

As it turns out we connect both frequencies to a gain node that has already been connected to the output:

http://is.gd/Bikhy9

This causes the two frequencies to start playing immediately and at slightly different times. I did a few measurements and as it turns out normally connecting the two different frequency nodes usually happens within 1-2ms of each other - which is inaudible to the listener - but it sometimes happens with a longer interval in-between (10s of ms) which is probably due to process/task scheduling under busy CPU conditions. This is likely to be perceived as a glitch (IIRC the ear can distinguish sounds that are 5ms apart).

I addressed the problem in this patch by changing the order in which we connect the nodes. First we create and connect the frequency nodes to the gain node, then we connect the gain node to the master node. This way when the sound starts playing all the nodes have been already connected together and are guaranteed to start at the same time.

Comment 14

[Etienne Segonzac (:etienne)]

Comment on attachment 832948 [details] [diff] [review]
[PATCH] Make the busy tone longer and clearer

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

Very impressive analysis and simple patch <3

Comment 15

[Gabriele Svelto [:gsvelto]]

Created attachment 8333861 [details] [review]
[PULL REQUEST] Make the busy tone longer and clearer

Thanks for the review! I'm waiting for Travis to turn green before merging:

https://travis-ci.org/mozilla-b2g/gaia/builds/14144462

Comment 16

[Gabriele Svelto [:gsvelto]]

Pushed to master, revision ae813ac2e0a3117f75f8bb9587c2b45d76f1b546

I'm testing this on v1.2 before pushing there too as it's not using the new Web Audio API and thus the issue there might be different than on master.

Comment 17

[Jason Smith [:jsmith]]

(In reply to Gabriele Svelto [:gsvelto] from comment #16)
> Pushed to master, revision ae813ac2e0a3117f75f8bb9587c2b45d76f1b546
> 
> I'm testing this on v1.2 before pushing there too as it's not using the new
> Web Audio API and thus the issue there might be different than on master.

Should we put NO_UPLIFT in the whiteboard to ensure this patch doesn't get automatically uplifted until we're sure that this is safe to push to 1.2?

Comment 18

[Gabriele Svelto [:gsvelto]]

(In reply to Jason Smith [:jsmith] from comment #17)
> Should we put NO_UPLIFT in the whiteboard to ensure this patch doesn't get
> automatically uplifted until we're sure that this is safe to push to 1.2?

Yes, I forgot about that. I'll clear it once I'll have finished my testing.

Comment 19

[Gabriele Svelto [:gsvelto]]

I did some analysis on v1.2 and there the sound glitches are much more pronounced than on master so we might need a dedicated v1.2 fix for it. I'll try to work on that this week and if we can't make it by the end of the v1.2 timeframe we can just merge the 6-seconds length part of my patch that already makes the busy tone much easier to spot even if it's a bit chirpy.

Comment 20

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/73f545424c82

Comment 21

[isabelrios]

Hi, 

Just checked this bug with David and here some feedback:

The first time calling to a busy number, the busy tone heard is ok, the improvement is notable. But if you try again to call to that number, the tone sounds a little bit worse. And, if you make another try, then it is even worse. The sound glitches appear again.

Tested with master 11/20 build:
Gecko-afdc772
Gaia-eeda629

Thanks

Comment 22

[Gabriele Svelto [:gsvelto]]

(In reply to Isabel Rios [:isabel_rios] from comment #21)
> The first time calling to a busy number, the busy tone heard is ok, the
> improvement is notable. But if you try again to call to that number, the
> tone sounds a little bit worse. And, if you make another try, then it is
> even worse. The sound glitches appear again.

That's pretty bad; I'll try to reproduce and re-open the bug. Which device did you use for testing? My tests were done on a hamachi.

Comment 23

[isabelrios]

Hi Gabriele,

I tested on unagi since the builds we have for hamachi are only 1.2 builds. Anyway, our release engineer is creating a master build for hamachi so I can test it also with that device. Will let you know when the tests are done.
Thanks

Comment 24

[Etienne Segonzac (:etienne)]

We might need loop in webaudio peeps too.

Comment 25

[isabelrios]

Hi Gabriele,

After checking with hamachi, what seen is that the busy tone sounds better and after several tries calling to a busy number it does not get worse. The only thing observed is that after calling to a busy number if a new try is made in a relative short time (2-3 seconds), only one busy tone is heard. Then, trying again, the busy tone is right. Could you reproduce this?

Thanks,
Isabel

Comment 26

[Gabriele Svelto [:gsvelto]]

I have not finished the v1.2 patch and did not have the time to reproduce the issue in comment 25 so I'm moving this to the next sprint.

Comment 27

[David Palomino [:dpalomino]]

Hi Gabriele, 

Thanks a lot Gabriele for taking care of this. 

In reply to Isabel Rios [:isabel_rios] from comment #25)
> After checking with hamachi, what seen is that the busy tone sounds better
> and after several tries calling to a busy number it does not get worse. The
> only thing observed is that after calling to a busy number if a new try is
> made in a relative short time (2-3 seconds), only one busy tone is heard.
> Then, trying again, the busy tone is right. Could you reproduce this?

Did you managed to reproduce this?

Please, let us know if we can help... 

Thanks!
David

Comment 28

[Gabriele Svelto [:gsvelto]]

Created attachment 8338208 [details] [diff] [review]
[PATCH v1.2 1/2] Make the busy tone longer and clearer

This is a fix for the v1.2 branch and since the audio API changed it doesn't even look remotely similar to the master fix hence I'm asking for review separately.

While testing I noticed that with the current setup sometimes a bunch of mozWriteAudio() calls would fail to write any frames at all (returning 0 for the number of written frames); these group of calls seemed to correspond to the audible glitches. After experimenting a bit I noticed that increasing the output buffer we use made the problem disappear so this patch uses a 50% larger buffers for long tones than for short ones.

I will also post a patch that allows this to be tested easily without having to make an actual call. I've tested this on an hamachi and I'll try and test it on an inari too to make sure it's working fine there too.

Comment 29

[Gabriele Svelto [:gsvelto]]

Created attachment 8338211 [details] [diff] [review]
Play the busy tone when calling an empty number

This is a quick and dirty patch to simplify testing the busy tone. It changes the dialer so that when the call button is pressed without a number it will play the busy tone instead of calling the last call in the log.

Comment 30

[Etienne Segonzac (:etienne)]

Comment on attachment 8338208 [details] [diff] [review]
[PATCH v1.2 1/2] Make the busy tone longer and clearer

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

All good, r=me

Comment 31

[Gabriele Svelto [:gsvelto]]

(In reply to Etienne Segonzac (:etienne) from comment #30)
> All good, r=me

Thanks for the review! I've tested it on my inari and it works fine there too.

Pushed to gaia/v1.2 413f2ae92710ffa4233119c1a68d20a04451472b

Comment 32

[Gabriele Svelto [:gsvelto]]

(In reply to David Palomino [:dpv] from comment #27)
> Did you managed to reproduce this?

I didn't have the time yet as I was working on the more urgent v1.2 fix. I'll be re-testing on master soon and re-open this bug if I can reproduce the problem (or maybe open a follow-up since the issue seems to be different).

Comment 33

[Gabriele Svelto [:gsvelto]]

Created attachment 8338259 [details] [diff] [review]
[PATCH v1.2 2/2] Lengthen the busy tone to 6 seconds r=etienne

Yuck, I forgot to uplift a piece of the original patch, this is what jetlag does to you :-( Anyway I'm flagging this as r=etienne as it's identical to the trivial change that was already in attachment 832948 [details] [diff] [review].

Comment 34

[Gabriele Svelto [:gsvelto]]

Pushed the missing piece to gaia/v1.2 264c6044b941437ac3c4b28fe4ca392d2bc78445