Closed Bug 997701 Opened 6 years ago Closed 6 years ago

[AudioContext] To use suitable audio channel type to AudioContext

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:1.3+, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S1 (9may)
blocking-b2g 1.3+
Tracking Status
b2g-v1.3 --- fixed
b2g-v1.3T --- fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed

People

(Reporter: scheng, Assigned: etienne)

References

Details

(Whiteboard: [cert])

Attachments

(2 files)

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).
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.
Flags: needinfo?(etienne)
mark 1.3? because bug 984498 is 1.3+ & block IOT issue.
Blocks: 984498
blocking-b2g: --- → 1.3?
Hi Etienne, since 994498 blocks the Spain IOT next Monday, please kindly help to check this one when you have time.
blocks a cert blocker
blocking-b2g: 1.3? → 1.3+
Component: Gaia → Gaia::Dialer
Whiteboard: [cert]
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?
(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.
Ya Ruili, once you modify and verify done, please let us know the result~

Thanks
Flags: needinfo?(zhang.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][vchen@mozilla.com] from comment #7)
> Ya Ruili, once you modify and verify done, please let us know the result~
> 
> Thanks
Flags: needinfo?(zhang.ruili)
(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][vchen@mozilla.com] 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
(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.
Flags: needinfo?(etienne)
Vance, we apply the patches you provide and it runs ok.
(In reply to Vance Chen [:vchen][vchen@mozilla.com] from comment #7)
> Ya Ruili, once you modify and verify done, please let us know the result~
> 
> Thanks
No longer blocks: 984498
Depends on: 984498
Blocks: 981885
(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
Flags: needinfo?(etienne)
Attached file Gaia PR
Star, can you confirm that this fixes the issue for you?

Gabriele, I think you're the best reviewer for this patch.
Attachment #8410979 - Flags: review?(gsvelto)
Attachment #8410979 - Flags: feedback?(scheng)
Flags: needinfo?(etienne)
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.
Attachment #8410979 - Flags: feedback?(scheng) → feedback+
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.
Flags: needinfo?(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)
Flags: needinfo?(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 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.
Attachment #8410979 - Flags: review?(gsvelto) → review+
(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.
(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. ^^
Assignee: nobody → 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?
Attachment #8410979 - Flags: review?(anthony)
(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 on attachment 8410979 [details] [review]
Gaia PR

Yup, happy to do so.
Attachment #8410979 - Flags: review?(anthony) → review+
https://github.com/mozilla-b2g/gaia/commit/3617fa6b6e5e2e3c7a198227d27a12bca830ff0a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Needs rebased patches for the v1.3/v1.4 uplifts.
Flags: needinfo?(etienne)
Target Milestone: --- → 2.0 S1 (9may)
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)
Flags: needinfo?(ryanvm)
Flags: needinfo?(etienne)
None of the above are things I have a say in.
Flags: needinfo?(ryanvm)
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.
Attachment #8416174 - Flags: review?(kgrandon)
Flags: needinfo?(etienne)
Comment on attachment 8416174 [details] [review]
Github PR, fix 1.3t unit test

R+ assuming tests pass. Thanks for finding this so quickly!
Attachment #8416174 - Flags: review?(kgrandon) → review+
Thanks guys!
You need to log in before you can comment on or make changes to this bug.