917196 - [wasabi][DTMF] When audio tone is set to short, user will still hear long audio tone if long press dailer keypad during a call.

Status: VERIFIED FIXED

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

Description

[Enpei]

Attachment: logcat

Key tone still plays as long as user presses the button even audio touch tone is set to short.

* Build Number                
Gaia:     c6b4cc05b2de6884a652c1c5ab8401216ffa46c1
Gecko:    78b3dbc50a8cddea792b6c2870c0bfbe3726335c
BuildID   20130917062051
Version   26.0a1

* Reproduce Steps
1. Go to Setting > Sound > audio touch tone. Set to short.
2. Make a call, after establishing, long press keypad.

* Expected Result
Tone can only play shortly even button is not released.

* Actual Result
Tone plays as long as the key is pressed.

* Occurrence rate
100%

Comment 1

[Francis Lee [:frlee]]

according to 9/25 triage result, change to koi+

Comment 2

[Ken Chang[:kenkai]]

Gabriele, please take this bug. Thanks.

Comment 3

[Steve Chung [:steveck]]

Hi Gabriele, since you raise the concern in the meeting about lack of device for developing, you can reassign this issue to me if it really trouble you, thanks.

Comment 4

[Gabriele Svelto [:gsvelto]]

(In reply to Steve Chung [:steveck] from comment #3)
> Hi Gabriele, since you raise the concern in the meeting about lack of device
> for developing, you can reassign this issue to me if it really trouble you,
> thanks.

I'd appreciate that. That being said my patch didn't change the audible tone, only the dial-tone which is what was requested. I had verified at the time that the dial-tones would be shortened to 120ms.

Comment 5

[Steve Chung [:steveck]]

Just checked with Enpei and UX, the audio tone here should be the sound displayed from device, and it's different from the dial tone which should be sent with telephony api. I've checked the code and dial tone is adjusted by DTMF settings, but the audio tone length is unchanged here. Basically there is no spec defines that length of audio tone, it's more likely an UX issue here(some Android device will sync the length of displayed audio tone and sent dial tone). Need info to UX team for this issue.

Another question here is about the long press duration. The sent dial tone length should be the same as press duration, but the tone duration always shorter than expect. The dial tone start/stop looks triggered at the correct timing. Hi HsinYi, do you have any thought about this result?

Comment 6

[Gabriele Svelto [:gsvelto]]

(In reply to Steve Chung [:steveck] from comment #5)
> I've checked the code and dial tone is adjusted by
> DTMF settings, but the audio tone length is unchanged here. Basically there
> is no spec defines that length of audio tone, it's more likely an UX issue
> here(some Android device will sync the length of displayed audio tone and
> sent dial tone). Need info to UX team for this issue.

Great, thanks, I'll wait for their feedback and prepare a patch in case we should change the audible tones too.

> Another question here is about the long press duration. The sent dial tone
> length should be the same as press duration, but the tone duration always
> shorter than expect. The dial tone start/stop looks triggered at the correct
> timing.

I haven't written that code but I could double-check it. In any case it shouldn't be too complicated to make the dialtone and audible tones the same length.

Comment 7

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

(In reply to Gabriele Svelto [:gsvelto] from comment #6)
> (In reply to Steve Chung [:steveck] from comment #5)
> > I've checked the code and dial tone is adjusted by
> > DTMF settings, but the audio tone length is unchanged here. Basically there
> > is no spec defines that length of audio tone, it's more likely an UX issue
> > here(some Android device will sync the length of displayed audio tone and
> > sent dial tone). Need info to UX team for this issue.
> 
> Great, thanks, I'll wait for their feedback and prepare a patch in case we
> should change the audible tones too.
> 
> > Another question here is about the long press duration. The sent dial tone
> > length should be the same as press duration, but the tone duration always
> > shorter than expect. The dial tone start/stop looks triggered at the correct
> > timing.
> 
> I haven't written that code but I could double-check it. In any case it
> shouldn't be too complicated to make the dialtone and audible tones the same
> length.

Just played with Android on CDMA. The situation is the same, the sent tone the remote party heard is shorter than expected.

I need to investigate more to know how modem or network controls the tone if we trigger startTone/stopTone commands at the right timing.

Comment 8

[Hsin-Yi Tsai (she/her) | away [:hsinyi]]

Just realized why the sent tone is shorter than expected.
[1] and [2] controls the tone duration going to be sent, and they conform with the user behavior. But [3] is always called right after [1] that stops the tone unexpectedly.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/keypad.js#L411
[2] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/keypad.js#L414
[3] https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/keypad.js#L425

Comment 9

[Neo Hsieh [:neo]]

Hi Steve, As we discussed, The audio tone should be the same sound displayed from devices.

Comment 10

[Gabriele Svelto [:gsvelto]]

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #8)
> Just realized why the sent tone is shorter than expected.
> [1] and [2] controls the tone duration going to be sent, and they conform
> with the user behavior. But [3] is always called right after [1] that stops
> the tone unexpectedly.

Good catch Hsin-Yi, with long dialtones [3] will always set a timeout of 0 thus cutting short the tone almost immediately. I wonder why that code was done that way in the first place, when I modified it I didn't want to change it because I didn't understand the reasoning behind it. I'll investigate a bit and see if it needs to be fixed.

(In reply to Neo Hsieh from comment #9)
> Hi Steve, As we discussed, The audio tone should be the same sound displayed
> from devices.

Sounds good, then I'll make sure the audible tone has the same duration of the dialtone. Thanks for your feedback!

Comment 11

[Enpei]

Update title that from DUT side, user can still hear long audio tone when setting is set to short. As for sent tone bug, let's track on bug 922569.

Comment 12

[bhavana bajaj [:bajaj]]

HI Gabriele,

Just a quick ping to see if there is an upcoming patch on this ? Will be helpful if you can help with an eta for resolving this blocker.

Comment 13

[Gabriele Svelto [:gsvelto]]

(In reply to bhavana bajaj [:bajaj] from comment #12)
> Just a quick ping to see if there is an upcoming patch on this ? Will be
> helpful if you can help with an eta for resolving this blocker.

I'm currently working on it, a patch is coming :-)

Comment 14

[Kai-Chih Hu [:khu]]

(In reply to Gabriele Svelto [:gsvelto] from comment #13)
> 
> I'm currently working on it, a patch is coming :-)

Awesome! Thanks, Gabriele. Do we know the possible target milestone? :-)

Comment 15

[Gabriele Svelto [:gsvelto]]

(In reply to Kevin Hu [:khu] from comment #14)
> Awesome! Thanks, Gabriele. Do we know the possible target milestone? :-)

I've got to finish bug 922569 first so that I can fix this bug after I've cleaned up the DTMF behavior. If I'm lucky I might do it by the end of sprint 3 (10/14 ~ 10/25) but worst case this will be certainly done during sprint 4 (10/28 ~ 11/8).

Comment 16

[Kai-Chih Hu [:khu]]

Thanks, Gabriele. I will set the target milestone to sprint 4 for now.

Comment 17

[bhavana bajaj [:bajaj]]

(In reply to Gabriele Svelto [:gsvelto] from comment #15)
> (In reply to Kevin Hu [:khu] from comment #14)
> > Awesome! Thanks, Gabriele. Do we know the possible target milestone? :-)
> 
> I've got to finish bug 922569 first so that I can fix this bug after I've
> cleaned up the DTMF behavior. If I'm lucky I might do it by the end of
> sprint 3 (10/14 ~ 10/25) but worst case this will be certainly done during
> sprint 4 (10/28 ~ 11/8).

Hi :gsvelto, given 922569 has been landed, any updates in this issue. IS that land going to help here ?

Comment 18

[Gabriele Svelto [:gsvelto]]

(In reply to bhavana bajaj [:bajaj] from comment #17)
> Hi :gsvelto, given 922569 has been landed, any updates in this issue. IS
> that land going to help here ?

Yes, the changes there were required for this. I plan to close this bug by the end of this week.

Comment 19

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH] When short tones are enabled make audible tones short too

The fix is fairly straightforward and I've added unit-tests covering this piece of functionality. I also took the liberty of fixing some (but not all) jshint errors.

Comment 20

[Gabriele Svelto [:gsvelto]]

Attachment: [PULL REQUEST] When short tones are enabled make audible tones short too

Comment 21

[Etienne Segonzac (:etienne)]

Comment on attachment 832345 [details] [diff] [review]
[PATCH] When short tones are enabled make audible tones short too

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

Looking good.
One thing: we should use |subject.init(true)| and |subject.init(false)| instead of modifying the |_oncall|.
And we should probably do it in symmetrical setup(s)() teardown(s)().

(I did a quick test it works well you just need to add an init method to the mock tone player).

Comment 22

[Gabriele Svelto [:gsvelto]]

Thanks for the review!

(In reply to Etienne Segonzac (:etienne) from comment #21)
> One thing: we should use |subject.init(true)| and |subject.init(false)|
> instead of modifying the |_oncall|.
> And we should probably do it in symmetrical setup(s)() teardown(s)().

You mean calling subject.init(true) within setup() and subject.init(false) within teardown()? Or do you mean we should add a teardown() function to the KeypadManager that undoes what init() does? (that would mostly mean de-registering various event-handlers)

Comment 23

[Gabriele Svelto [:gsvelto]]

Attachment: [PATCH v2] When short tones are enabled make audible tones short too

I've refactored the tests to take your comments into account. I've also reorganized the existing suites in order for the tests to share the same setup: I've introduced a suite for audible/DTMF tones when typing a number (not on a call) and another one for audible/DTMF tones when on a call. All the tests that were in the previous ones (one for audible and the other for DTMF tones) have been split among those two new suites.

Comment 24

[Kai-Chih Hu [:khu]]

Since 1.2 C4 was passed, could you assign a new target milestone that you're trying to hit?

Comment 25

[Gabriele Svelto [:gsvelto]]

(In reply to Kevin Hu [:khu] from comment #24)
> Since 1.2 C4 was passed, could you assign a new target milestone that you're
> trying to hit?

Right, I forgot to do that. I've moved it to C5 since I've already got a patch and it's likely to land on Monday.

Comment 26

[Etienne Segonzac (:etienne)]

Comment on attachment 832896 [details] [diff] [review]
[PATCH v2] When short tones are enabled make audible tones short too

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

All good, thanks!

Comment 27

[Gabriele Svelto [:gsvelto]]

Thanks for the review!

Pushed to gaia/master 04af73de791b4c5bb9b252f53d1b849c8ea167b8 and gaia/v1.2 4b6c51d1ea676bc812b4b2a146aef14865061b67

Comment 28

[Enpei]

Verified. 
Gaia:     8ff13cf43d185104a000bab68f5eef589b5d8684
Gecko:    41e8c46cb1b8b9b3058b1cbf6525c020bebfc4f0
BuildID   20131120031537
Version   26.0