Last Comment Bug 746496 - B2G telephony: update the audio system for incoming calls and holding calls
: B2G telephony: update the audio system for incoming calls and holding calls
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: ARM Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Hsin-Yi Tsai [:hsinyi]
:
Mentors:
Depends on:
Blocks: b2g-telephony
  Show dependency treegraph
 
Reported: 2012-04-18 00:18 PDT by Hsin-Yi Tsai [:hsinyi]
Modified: 2012-05-15 06:30 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Update the audio system as call changes (6.28 KB, text/plain)
2012-04-23 06:19 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details
Update the audio system as call changes (v2) (4.19 KB, patch)
2012-04-23 06:22 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Splinter Review
Patch (v3) (8.43 KB, patch)
2012-04-25 23:53 PDT, Hsin-Yi Tsai [:hsinyi]
no flags Details | Diff | Splinter Review
Patch (v4) (13.94 KB, patch)
2012-05-02 22:47 PDT, Hsin-Yi Tsai [:hsinyi]
philipp: review+
Details | Diff | Splinter Review

Description Hsin-Yi Tsai [:hsinyi] 2012-04-18 00:18:34 PDT
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 [:hsinyi] 2012-04-19 23:24:38 PDT
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 [:hsinyi] 2012-04-23 06:19:50 PDT
Created attachment 617452 [details]
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 [:hsinyi] 2012-04-23 06:22:41 PDT
Created attachment 617453 [details] [diff] [review]
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 [:hsinyi] 2012-04-23 07:16:17 PDT
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 [:hsinyi] 2012-04-25 23:53:40 PDT
Created attachment 618573 [details] [diff] [review]
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] 2012-04-27 16:27:25 PDT
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 [:hsinyi] 2012-04-29 19:42:04 PDT
(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 [:hsinyi] 2012-05-02 22:47:40 PDT
Created attachment 620596 [details] [diff] [review]
Patch (v4)

The updated patch addresses the comments above. Thanks!
Comment 9 Philipp von Weitershausen [:philikon] 2012-05-14 11:57:08 PDT
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] 2012-05-14 21:14:38 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e002bf20e8b3
Comment 11 Ed Morley [:emorley] 2012-05-15 06:30:28 PDT
https://hg.mozilla.org/mozilla-central/rev/e002bf20e8b3

Note You need to log in before you can comment on or make changes to this bug.