Closed
Bug 924454
Opened 12 years ago
Closed 12 years ago
Improve busy tone implementation
Categories
(Firefox OS Graveyard :: Gaia::Dialer, defect)
Tracking
(blocking-b2g:koi+, b2g18 wontfix, b2g-v1.1hd wontfix, b2g-v1.2 fixed)
People
(Reporter: brg, Assigned: gsvelto)
Details
Attachments
(5 files)
2.99 KB,
patch
|
etienne
:
review+
|
Details | Diff | Splinter Review |
46 bytes,
text/x-github-pull-request
|
Details | Review | |
2.33 KB,
patch
|
etienne
:
review+
|
Details | Diff | Splinter Review |
1.05 KB,
patch
|
Details | Diff | Splinter Review | |
1.37 KB,
patch
|
Details | Diff | Splinter Review |
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 3•12 years ago
|
||
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 ?
Assignee | ||
Comment 4•12 years ago
|
||
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.
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: needinfo?(gsvelto)
Resolution: --- → DUPLICATE
Reporter | ||
Comment 5•12 years ago
|
||
(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.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Reporter | ||
Comment 6•12 years ago
|
||
(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.
Updated•12 years ago
|
Assignee: nobody → gsvelto
Target Milestone: --- → 1.2 C4(Nov8)
Comment 7•12 years ago
|
||
Renoming for more discussion - this might need to be punted to 1.3 at this point.
blocking-b2g: koi+ → koi?
Reporter | ||
Comment 8•12 years ago
|
||
(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.
Assignee | ||
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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
Updated•12 years ago
|
Target Milestone: 1.2 C4(Nov8) → 1.2 C5(Nov22)
Comment 12•12 years ago
|
||
Hi,
After a talk with Gabriele (thanks a lot Gabriele), he confirmed he was taking care of this.
Thanks!
David
Assignee | ||
Comment 13•12 years ago
|
||
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.
Attachment #832948 -
Flags: review?(etienne)
Comment 14•12 years ago
|
||
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
Attachment #832948 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 15•12 years ago
|
||
Thanks for the review! I'm waiting for Travis to turn green before merging:
https://travis-ci.org/mozilla-b2g/gaia/builds/14144462
Assignee | ||
Comment 16•12 years ago
|
||
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.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
status-b2g18:
--- → wontfix
status-b2g-v1.1hd:
--- → wontfix
status-b2g-v1.2:
--- → affected
Resolution: --- → FIXED
Comment 17•12 years ago
|
||
(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?
Assignee | ||
Comment 18•12 years ago
|
||
(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.
Whiteboard: [NO_UPLIFT]
Assignee | ||
Comment 19•12 years ago
|
||
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 21•12 years ago
|
||
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
Assignee | ||
Comment 22•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
We might need loop in webaudio peeps too.
Comment 25•12 years ago
|
||
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
Assignee | ||
Comment 26•12 years ago
|
||
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.
Target Milestone: 1.2 C5(Nov22) → 1.2 C6(Dec6)
Comment 27•12 years ago
|
||
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
Assignee | ||
Comment 28•12 years ago
|
||
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.
Attachment #8338208 -
Flags: review?(etienne)
Assignee | ||
Comment 29•12 years ago
|
||
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•12 years ago
|
||
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
Attachment #8338208 -
Flags: review?(etienne) → review+
Assignee | ||
Comment 31•12 years ago
|
||
(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
Assignee | ||
Comment 32•12 years ago
|
||
(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).
Assignee | ||
Comment 33•12 years ago
|
||
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].
Assignee | ||
Updated•12 years ago
|
Attachment #8338208 -
Attachment description: [PATCH v1.2] Make the busy tone longer and clearer → [PATCH v1.2 1/2] Make the busy tone longer and clearer
Assignee | ||
Comment 34•12 years ago
|
||
Pushed the missing piece to gaia/v1.2 264c6044b941437ac3c4b28fe4ca392d2bc78445
Assignee | ||
Updated•12 years ago
|
Whiteboard: [NO_UPLIFT]
You need to log in
before you can comment on or make changes to this bug.
Description
•