997701 - [AudioContext] To use suitable audio channel type to AudioContext

Status: RESOLVED FIXED

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

Description

[Star Cheng [:scheng]]

Because DTMF tone/waiting tone/busy tone cannot be muted while in call, we have to use "telephony" this kind of audio channel type to ensure that. There is a issue about AudioChannelType of AudioContext, please reference Bug 984498. We fix the audio channel type issue, and then we need the help from gaia team member to resolve Bug 984498 totally. Please see below audio channel type of request:

DTMF tone: Normal (not in call) / Telephony (in call)
waiting tone/busy tone:  Telephony

There is a note: the audio channel type of AudioContext can be set in new AudioContext('telephony'), but can *not* set after creating the AudioContext(). Because there is not support in the Audio back-end to change the audiostream type (mapping to audio channel type).

Comment 1

[Star Cheng [:scheng]]

Hi,  Etienne

Could you help to use suitable AudioChannelType to TonePlayer in tone_player.js? If there is any problem, please feel free to contact with me.

Comment 2

[Star Cheng [:scheng]]

mark 1.3? because bug 984498 is 1.3+ & block IOT issue.

Comment 3

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Hi Etienne, since 994498 blocks the Spain IOT next Monday, please kindly help to check this one when you have time.

Comment 4

[Jason Smith [:jsmith]]

blocks a cert blocker

Comment 5

[RuiLi]

Hi Star, the 'new AudioContext()' is in the function ensureAudio() of tone_player.js, I see that. You suggest 'this._audioContext = new AudioContext()' be set as "this._audioContext = new AudioContext('telephony')", is it?

Comment 6

[Star Cheng [:scheng]]

(In reply to RuiLi from comment #5)
> Hi Star, the 'new AudioContext()' is in the function ensureAudio() of
> tone_player.js, I see that. You suggest 'this._audioContext = new
> AudioContext()' be set as "this._audioContext = new
> AudioContext('telephony')", is it?

I have explained the reason in the mail to Vance. AFAIK, Vance sent to the mail to you.

Comment 7

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

Ya Ruili, once you modify and verify done, please let us know the result~

Thanks

Comment 8

[RuiLi]

Vance, you sent mail to me today? I did not get it. Could you send it again? Thank you.
(In reply to Vance Chen [:vchen][[e-mail]] from comment #7)
> Ya Ruili, once you modify and verify done, please let us know the result~
> 
> Thanks

Comment 9

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to RuiLi from comment #8)
> Vance, you sent mail to me today? I did not get it. Could you send it again?
> Thank you.
> (In reply to Vance Chen [:vchen][[e-mail]] from comment #7)
> > Ya Ruili, once you modify and verify done, please let us know the result~
> > 
> > Thanks

Hi Ruili -

I sent you the patches last Friday afternoon 4:30 pm...anyway I just resend it again, please let me know if you still cannot find it

Thanks

Vance

Comment 10

[Etienne Segonzac (:etienne)]

(In reply to RuiLi from comment #5)
> Hi Star, the 'new AudioContext()' is in the function ensureAudio() of
> tone_player.js, I see that. You suggest 'this._audioContext = new
> AudioContext()' be set as "this._audioContext = new
> AudioContext('telephony')", is it?

We do use a setChannel method on the TonePlayer, so this will not be enough.
We should change the setChannel for something that does
```
this.trashAudio();
this._channel = the-channel'
this.ensureAudio()
```

and do a
```
this._audioContext = new AudioContext(this._channel);
```

in the ensureAudio method.

Comment 11

[RuiLi]

Vance, we apply the patches you provide and it runs ok.
(In reply to Vance Chen [:vchen][[e-mail]] from comment #7)
> Ya Ruili, once you modify and verify done, please let us know the result~
> 
> Thanks

Comment 12

[Vance Chen [:vchen][skype:vance.lucida][[e-mail]]]

(In reply to Etienne Segonzac (:etienne) from comment #10)
> (In reply to RuiLi from comment #5)
> > Hi Star, the 'new AudioContext()' is in the function ensureAudio() of
> > tone_player.js, I see that. You suggest 'this._audioContext = new
> > AudioContext()' be set as "this._audioContext = new
> > AudioContext('telephony')", is it?
> 
> We do use a setChannel method on the TonePlayer, so this will not be enough.
> We should change the setChannel for something that does
> ```
> this.trashAudio();
> this._channel = the-channel'
> this.ensureAudio()
> ```
> 
> and do a
> ```
> this._audioContext = new AudioContext(this._channel);
> ```
> 
> in the ensureAudio method.

Thanks for your feedback Etienne. As Star mentioned in Comment#1, could you kindly help to provide the patches that use the suitable channel type in Gaia side?

Thanks

Vance

Comment 13

[Etienne Segonzac (:etienne)]

Attachment: Gaia PR

Star, can you confirm that this fixes the issue for you?

Gabriele, I think you're the best reviewer for this patch.

Comment 14

[Star Cheng [:scheng]]

Comment on attachment 8410979 [details] [review]
Gaia PR


It's reasonable to me. 
And I have question: How to handle "_channel" when invoking trashAudio()? 

A user story: 
1.User-A use TonePlayer.Init('telephony'), and after finish doing something and then call TonePlayer.trashAudio() to free _audioContext.

2.User-B user TonePlayer.Init('telephony') 

In this case, I guess "if (!channel || this._channel === channel)" is true, and we get return. So we can not get another AudioContext node from this.ensureAudio(). And in the mean time, the _audioContext is null, so we can not use TonePlayer correctly.

Comment 15

[Star Cheng [:scheng]]

Hi, Etienne

Do you know who can help to modify system tone & busy tone in Gaia layer? I also need the patch to resolve Bug 984498.

Comment 16

[Etienne Segonzac (:etienne)]

(In reply to Star Cheng [:scheng] from comment #14)
> In this case, I guess "if (!channel || this._channel === channel)" is true,
> and we get return. So we can not get another AudioContext node from
> this.ensureAudio(). And in the mean time, the _audioContext is null, so we
> can not use TonePlayer correctly.

You saved me :)
(the patch is now updated to nullify _channel when we trash the audio context)

Comment 17

[Etienne Segonzac (:etienne)]

(In reply to Star Cheng [:scheng] from comment #15)
> Hi, Etienne
> 
> Do you know who can help to modify system tone & busy tone in Gaia layer? I
> also need the patch to resolve Bug 984498.

The busy tone is using the same TonePlayer that we're modifying with this patch.
Not sure what the system tone is.

Comment 18

[Gabriele Svelto [:gsvelto]]

Comment on attachment 8410979 [details] [review]
Gaia PR

LGTM with the nit that I'd like this covered with unit-tests before we land. Something able to catch the issue Star noticed in comment 14 would be excellent to make sure we don't accidentally regress the init()/trash() cycles.

Comment 19

[Star Cheng [:scheng]]

(In reply to Etienne Segonzac (:etienne) from comment #17)
> (In reply to Star Cheng [:scheng] from comment #15)
> > Hi, Etienne
> > 
> > Do you know who can help to modify system tone & busy tone in Gaia layer? I
> > also need the patch to resolve Bug 984498.
> 
> The busy tone is using the same TonePlayer that we're modifying with this
> patch.
> Not sure what the system tone is.


Sorry! It's my typo. It should be waiting tone instead of system tone.

Comment 20

[Star Cheng [:scheng]]

(In reply to Etienne Segonzac (:etienne) from comment #16)
> (In reply to Star Cheng [:scheng] from comment #14)
> > In this case, I guess "if (!channel || this._channel === channel)" is true,
> > and we get return. So we can not get another AudioContext node from
> > this.ensureAudio(). And in the mean time, the _audioContext is null, so we
> > can not use TonePlayer correctly.
> 
> You saved me :)
> (the patch is now updated to nullify _channel when we trash the audio
> context)

It's cool. I am looking forward to kill the bug 984498. ^^

Comment 21

[Etienne Segonzac (:etienne)]

Comment on attachment 8410979 [details] [review]
Gaia PR

Updated PR with tests currently running on Travis.

Rik, I asked Gabriele for review since he knows the TonePlayer better than any dialer peer, but would you like to stamp this?

Comment 22

[Gabriele Svelto [:gsvelto]]

(In reply to Etienne Segonzac (:etienne) from comment #21)
> Updated PR with tests currently running on Travis.

<3 the new unit-tests. One of Travis jobs had failed but it was an intermittent, I re-triggered it and now it's green.

Comment 23

[Anthony Ricaud (:rik)]

Comment on attachment 8410979 [details] [review]
Gaia PR

Yup, happy to do so.

Comment 24

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/3617fa6b6e5e2e3c7a198227d27a12bca830ff0a

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Needs rebased patches for the v1.3/v1.4 uplifts.

Comment 26

[Etienne Segonzac (:etienne)]

Landed on v1.4
https://github.com/mozilla-b2g/gaia/commit/c90ebfcb79b75ee230e9a48bcfb64cdcc75d293f

Comment 27

[Etienne Segonzac (:etienne)]

Landed on 1.3
https://github.com/mozilla-b2g/gaia/commit/667539f1ed4becc45b182a5f1046221d3eeb9e7c

Comment 28

[Kevin Grandon :kgrandon]

Uplifting this into 1.3t has caused travis to be perma-red: https://travis-ci.org/mozilla-b2g/gaia/builds/24208155

This is probably broken on 1.3, but we can't tell because travis is currently perma-grey. We are currently deciding if it's worth debugging further, or if the ship has already sailed there. It's probably worth it to fix this on v1.3t though. Do we want to:

1 - Backout the merge to 1.3t
2 - Have someone fix the failing test on 1.3t
3 - Disable the test on 1.3t

(As 1.3t is currently red, we should probably close the tree until we fix or revert this)

Comment 29

[Ryan VanderMeulen [:RyanVM]]

None of the above are things I have a say in.

Comment 30

[Michael Henretty [:mikehenrty][:mhenretty]]

Attachment: Github PR, fix 1.3t unit test

The problem is that tone_player.js is in the shared directory for 1.3t (because call screen lives in system now presumable. Just had to fix the import.

Comment 31

[Kevin Grandon :kgrandon]

Comment on attachment 8416174 [details] [review]
Github PR, fix 1.3t unit test

R+ assuming tests pass. Thanks for finding this so quickly!

Comment 32

[Kevin Grandon :kgrandon]

Landed follow-up, thanks for the quick fix. https://github.com/mozilla-b2g/gaia/commit/a8e0ff550de08e58e4bf75af3cecf175b9b71e70

Comment 33

[Etienne Segonzac (:etienne)]

Thanks guys!