746496 - B2G telephony: update the audio system for incoming calls and holding calls

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

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

We need to do some work for updating the audio system for holding calls. Also, some modification is needed to update the audio correctly when there's an incoming call.

Comment 1

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

I just realized that it fails in assigning value to |gAudioManager.phoneState| in updateCallAudioState(), RadioInterfaceLayer.js. 

It also seems that gAudioManager.setForceForUse() doesn't work because nothing changes even after I marked this.

Comment 2

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

Attachment: Update the audio system as call changes

The audio state should be updated with the state of *activeCall*. This patch updates *activeCall* and the audio state for holding calls and multiple calls. At any given time, there is only one *activeCall* even we support multiple. 

Also, as I mentioned in Comment 1, I found it fails in assigning a value to |gAudioManager.phoneState|. 
I.e. the value of |gAudioManager.phoneState| is the same no matter what we assign. 
However, when I marked and unmarked the line,
   gAudioManager.phoneState = nsIAudioManager.PHONE_STATE_IN_CALL;
in RadioInterfaceLayer.js, the audio system acted differently, even the assignment failed. I have no idea why. 
And I am also wondering if this is out of the scope of this bug? 
Would it be better to file a new bug for deeper discussion, if this is really a bug? 
Thanks!

Comment 3

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

Attachment: Update the audio system as call changes (v2)

Update the audio system as call changes

The audio state should be updated with the state of *activeCall*. This patch updates *activeCall* and the audio state for holding calls and multiple calls. At any given time, there is only one *activeCall* even we support multiple. 

Also, as I mentioned in Comment 1, I found it fails in assigning a value to |gAudioManager.phoneState|. 
I.e. the value of |gAudioManager.phoneState| is the same no matter what we assign. 
However, when I marked and unmarked the line,
   gAudioManager.phoneState = nsIAudioManager.PHONE_STATE_IN_CALL;
in RadioInterfaceLayer.js, the audio system acted differently, even the assignment failed. I have no idea why. 
And I am also wondering if this is out of the scope of this bug? 
Would it be better to file a new bug for deeper discussion, if this is really a bug? 
Thanks!

Comment 4

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

Comment on attachment 617453 [details] [diff] [review]
Update the audio system as call changes (v2)

Sorry for canceling the request for review. I just thought of a better implementation and would like to submit a update later. Thanks :)

Comment 5

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

Attachment: Patch (v3)

The audio state should be updated with the state of *activeCall*. This patch updates *activeCall* and the audio state for holding calls and multiple calls. At any given time, there is only one *activeCall* even we support multiple. 

This patch also fixes the broken setter of |gAudioManager.phoneState|. So the incorrect assignment I mentioned in Comment 3 doesn't exist.

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 618573 [details] [diff] [review]
Patch (v3)

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

I love that we're centralizing the active call computation in one place now. However, it's not quite consistent with how `Telephony.cpp` computes the active call [1]. We might want to update `Telephony.cpp` to use the same logic that you implemented in `ril_worker.js` now. Also, we might as well update `RadioInterfacdeLayer.handleEnumerateCalls()` and the corresponding code in `RILContentHelper` to simply use `call.isActive` rather than comparing indexes.

[1] https://mxr.mozilla.org/mozilla-central/source/dom/telephony/Telephony.cpp#447

::: dom/system/gonk/AudioManager.cpp
@@ +166,5 @@
>    if (AudioSystem::setPhoneState(aState)) {
>      return NS_ERROR_FAILURE;
>    }
> +
> +  mPhoneState = aState;

Good catch!

::: dom/system/gonk/ril_worker.js
@@ +1707,4 @@
>            currentCall.state = newCall.state;
> +          this.currentCallsLength = newCalls.length;
> +          currentCall.isActive =
> +            this._isActiveCall(currentCall.state, this.currentCallsLength);

If all you're doing is passing `newCalls.length` along to _isActiveCall, why even store it in `this.currentCallsLength`? Also, you're doing the storing in a loop... Could just do it once at the beginning, no? (With the right guards against `newCalls` of course.)

Comment 7

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

(In reply to Philipp von Weitershausen [:philikon] from comment #6)
> Comment on attachment 618573 [details] [diff] [review]
> Patch (v3)
> 
> Review of attachment 618573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I love that we're centralizing the active call computation in one place now.
> However, it's not quite consistent with how `Telephony.cpp` computes the
> active call [1]. We might want to update `Telephony.cpp` to use the same
> logic that you implemented in `ril_worker.js` now. Also, we might as well
> update `RadioInterfacdeLayer.handleEnumerateCalls()` and the corresponding
> code in `RILContentHelper` to simply use `call.isActive` rather than
> comparing indexes.
> 
Yap, you are right. Thanks for pointing this out. I shall revise those as well. 

> 
> ::: dom/system/gonk/ril_worker.js
> @@ +1707,4 @@
> >            currentCall.state = newCall.state;
> > +          this.currentCallsLength = newCalls.length;
> > +          currentCall.isActive =
> > +            this._isActiveCall(currentCall.state, this.currentCallsLength);
> 
> If all you're doing is passing `newCalls.length` along to _isActiveCall, why
> even store it in `this.currentCallsLength`? Also, you're doing the storing
> in a loop... Could just do it once at the beginning, no? (With the right
> guards against `newCalls` of course.)
Agree, there could be a better way to implementing this block. Will re-implement this. Thanks~

Comment 8

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

Attachment: Patch (v4)

The updated patch addresses the comments above. Thanks!

Comment 9

[Philipp von Weitershausen [:philikon]]

Comment on attachment 620596 [details] [diff] [review]
Patch (v4)

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

Awesome!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +503,3 @@
>      for (let i in calls) {
>        calls[i].state = convertRILCallState(calls[i].state);
>      }

You know, we could even do this in ril_worker.js, then the mainthread doesn't have to do anything special anymore... Not super necessary, just a thought.

::: dom/system/gonk/ril_worker.js
@@ +1830,5 @@
> +      case CALL_STATE_INCOMING:
> +      case CALL_STATE_DIALING:
> +      case CALL_STATE_ALERTING:
> +      case CALL_STATE_ACTIVE:
> +        return true;

I'm excited about this because I think it will make the API saner (I discovered the inconsistencies you're fixing here while writing tests...)

Comment 10

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e002bf20e8b3

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/e002bf20e8b3