859354 - Not alerting tone during an established call (call waiting)

Status: VERIFIED FIXED

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

Description

[David Palomino [:dpalomino]]

During a voice call, being Call Waiting enabled, alerting tone is not reproduced when receiving a second voice call (just a vibration alert is performed). 

This bug has been reported as blocking issue. 

Steps to reproduce: 

1. Establish voice call
2. Call Waiting enabled
3. Receive a second call 

Current: 
4. Vibrating alert (only) to users, indicating second call

Expected: 

4. Alerting tone (i.e. pi-pi pi-pi) should be reproduced. 


Additional note: 

http://developer.android.com/reference/android/media/ToneGenerator.html

public static final int TONE_SUP_CALL_WAITING

Added in API level 1
Call supervisory tone, Call Waiting: CEPT, JAPAN: 425Hz, 200ms ON, 600ms OFF, 200ms ON, 3s OFF... ANSI (IS-95): 440 Hz, 300 ms ON, 9.7 s OFF, (100 ms ON, 100 ms OFF, 100 ms ON, 9.7s OFF ...)


This tone is superimposed on the audio traffic received by the called user.

Comment 1

[Daniel Coloma:dcoloma]

CCing Etienne as I believe he is familiar Whitehead the call waitng code.

Comment 2

[Juan Perez-Bedmar [:juanpbf]]

adding some colleagues in CC

Comment 3

[Kai-Chih Hu [:khu]]

Etienne, is this something you could help? Or?

Comment 4

[Etienne Segonzac (:etienne)]

(In reply to khu from comment #3)
> Etienne, is this something you could help? Or?

It's on my list for this afternoon.
If I confirm it can be done on the gaia side I'll take the bug.

Comment 5

[Etienne Segonzac (:etienne)]

Attachment: Patch proposal

Apparently the default ('normal') audio channel nicely plays the alerting tone on top of the telephony channel.

Comment 6

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

Hi guys! No need to say that Etienne's patch works perfectly but I have several questions ;-)

In the current patch proposed by Etienne:
1. The "Vibrate" status in Settings is not considered at all. Should it be considered or should the device always vibrate in presence of a second call?
2. I guess neither the tone played (https://github.com/mozilla-b2g/gaia/blob/master/shared/resources/media/notifications/notifier_bap.opus) nor its ON/OFF duration are the final ones. Am I right, Etienne? In fact, David, from comment 1, should we be playing a 440 Hz tone during 300ms and then nothing for 9.7 seconds?

Thanks!

Comment 7

[Etienne Segonzac (:etienne)]

(In reply to gtorodelvalle from comment #6)
> Hi guys! No need to say that Etienne's patch works perfectly but I have
> several questions ;-)
> 
> In the current patch proposed by Etienne:
> 1. The "Vibrate" status in Settings is not considered at all. Should it be
> considered or should the device always vibrate in presence of a second call?

FTR the current code always vibrates because we had only a vibration signal, easily changeable.

> 2. I guess neither the tone played
> (https://github.com/mozilla-b2g/gaia/blob/master/shared/resources/media/
> notifications/notifier_bap.opus) nor its ON/OFF duration are the final ones.
> Am I right, Etienne?

Well if the specific tone is regulated and mcc-dependant maybe this shouldn't be handled by gaia but by the RIL...

Comment 8

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

I absolutely agree with Etienne and in fact I am suffering it on my skin because of bug 857951 :-p Let's see what David has to tell us about it and if we should raise it ;-)

Comment 9

[David Palomino [:dpalomino]]

Hi!

> In the current patch proposed by Etienne:
> 1. The "Vibrate" status in Settings is not considered at all. Should it be
> considered or should the device always vibrate in presence of a second call?

Yes, other reference handsets do not implement vibration (which is more associated with message reception)

> 2. I guess neither the tone played
> (https://github.com/mozilla-b2g/gaia/blob/master/shared/resources/media/
> notifications/notifier_bap.opus) nor its ON/OFF duration are the final ones.
> Am I right, Etienne? In fact, David, from comment 1, should we be playing a
> 440 Hz tone during 300ms and then nothing for 9.7 seconds?
> 
That's what it was requested, and also Android's implementation. Maybe I'm wrong, this is mcc/mnc independent. 

Thks!
David


> Thanks!

Comment 10

[Etienne Segonzac (:etienne)]

Hey, should we really be implementing this bug and bug 857951 in gaia?

Comment 11

[José Antonio Olivera Ortega [:jaoo]]

(In reply to Etienne Segonzac (:etienne) from comment #10)
> Hey, should we really be implementing this bug and bug 857951 in gaia?

Let me jump into the discussion guys. After a bit of researching about bug 857951 I figured out that this issue is handled in Android out of the RIL plumbing. The RIL must to notify to content about changes and states in the call (as It does) and then the phone app must lead with all these tones. See [1] please.

[1] https://android.googlesource.com/platform/packages/apps/Phone/+/master/src/com/android/phone/CallNotifier.java

Comment 12

[Etienne Segonzac (:etienne)]

(In reply to José Antonio Olivera Ortega [:jaoo] from comment #11)
> (In reply to Etienne Segonzac (:etienne) from comment #10)
> > Hey, should we really be implementing this bug and bug 857951 in gaia?
> 
> Let me jump into the discussion guys. After a bit of researching about bug
> 857951 I figured out that this issue is handled in Android out of the RIL
> plumbing. The RIL must to notify to content about changes and states in the
> call (as It does) and then the phone app must lead with all these tones. See
> [1] please.
> 

:/

German, let's sync up, this bug and bug 857951 should not be worked on independently.

Comment 13

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

Damn it! English sucks :p Sorry! Is your last comment a question? Or do you suggest to solve both bugs jointly? :)

Comment 14

[Etienne Segonzac (:etienne)]

(In reply to gtorodelvalle from comment #13)
> Damn it! English sucks :p Sorry! 

My bad :)

> Is your last comment a question? Or do you
> suggest to solve both bugs jointly? :)

Yes, that's my suggestion.

Comment 15

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

(no, no, not your English... My English :-p)

Perfect! Let me finish my tests and I'll share the .patch with you.

Comment 16

[Etienne Segonzac (:etienne)]

Attachment: Pointer to gaia PR

Here is a new version following the correct ANSI spec.
We play the done with a flexible method that I think you're going to like:

- The TonePlayer got promoted and now has it's own file
- in order to deserve this promotion it got a new `playSequence` method
- We use it to play the proper tone, and we'll be able to use it for other tones too!

I also added the check to prevent vibration if the setting says otherwise.

Comment 17

[Etienne Segonzac (:etienne)]

German did you get a chance to look at it?

Comment 18

[Etienne Segonzac (:etienne)]

Attachment: Pointer to updated gaia PR

Udpated !

Comment 19

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

(In reply to Etienne Segonzac (:etienne) from comment #17)
> German did you get a chance to look at it?

Ups! I didn't see your comment. Sorry! :-)

Comment 20

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/0cda546da5a2d455f95cf02a189f5f56ab0a5be5

Comment 21

[James Lal [:lightsofapollo]]

I was not able to uplift this bug to v1-train and v1.0.1.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train and v1.0.1, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 0cda546da5a2d455f95cf02a189f5f56ab0a5be5
  <RESOLVE MERGE CONFLICTS>
  git commit
  git checkout v1.0.1
  git cherry-pick -x $(git log -n1 v1-train --pretty=%H)

Comment 22

[Etienne Segonzac (:etienne)]

Attachment: Patch rebased on v1-train

Hope it helps, let me know!

Comment 23

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

Hi James! Is Etienne's comment 22 enough for the uplifting?

The harry is due that the patch for bug 857951 has succesfully landed in v1-train and v1.0.1 but it highly depends on this one so it will not work until this also lands. Thanks!

Comment 24

[James Lal [:lightsofapollo]]

Using etienne's patch:

in v1-train: 17ee28d556ccf82324c0b0c6216cd8145bf73a5a
in v1.0.1: f9bf2a2db13d4203753588e3e905f7ce8cb2cfa9

Comment 25

[John Hammink]

Currently, khepera_433360 says (and tests on 5/13, 5/16 partner builds show) that we're getting a tone: "The real behaviour I've seen with B02 version is that there IS an alerting tone, and it is repeated every 7seconds, aprox. So, although the separation between tones might be excesive, it is working."

Enough to verify this fix?