931722 - [Fugu]Can't mute in the call

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[sam.hua]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.22 (KHTML, like Gecko) Ubuntu Chromium/25.0.1364.160 Chrome/25.0.1364.160 Safari/537.22

Steps to reproduce:

1. setup a call.
2. press mute icon


Actual results:

The mic phone couldn't be muted


Expected results:

The mic phone should be muted

Comment 1

[sam.hua]

In TelephonyProvider.js
set microphoneMuted() will call "gAudioManager.microphoneMuted = aMuted",but our tara device fails to mute the mic.

In android, there are two modules to handle mic muting,audio and RIL. Tara device works well if apps choose the RIL.

Could TelephonyProvider in Gecko also support the muting operation in the RIL module? Sending "setMicMute" message to ril_worker and ril_worker sends the "REQUEST_SET_MUTE" to rild.

Comment 2

[James Zhang (Spreadtrum)]

Ming, pls add the patch.

Comment 3

[Ming]

Attachment: patch for this bug, route the mute to RIL

In TelephonyProvider.js ,we route the request of "mute the mic" to RIL, not to AudioManger.

Referd to ANDROID , it send the request to RIL, not to AudioManager ,actually the reason is that the value of parameter of "send_mic_mute_to_AudioManager"  is false.

Comment 4

[James Zhang (Spreadtrum)]

Add Ken

Comment 5

[Steven Yang [:styang]]

Can't reproduce it in Buri with v1.2.

Comment 6

[James Zhang (Spreadtrum)]

(In reply to styang from comment #5)
> Can't reproduce it in Buri with v1.2.

Maybe it's chipset hardcode.
Because our 7710 android bsp version is also ok.

Comment 7

[shawn ku [:sku]]

(In reply to James Zhang from comment #6)
> (In reply to styang from comment #5)
> > Can't reproduce it in Buri with v1.2.
> 
> Maybe it's chipset hardcode.
> Because our 7710 android bsp version is also ok.

This feature is chipset dependent.
Some chipset need mute request from audioManager, some need from RIL.


The information should be provided by chipset provider.

thanks!!
sku

Comment 8

[James Zhang (Spreadtrum)]

(In reply to shawn ku [:sku] from comment #7)
> (In reply to James Zhang from comment #6)
> > (In reply to styang from comment #5)
> > > Can't reproduce it in Buri with v1.2.
> > 
> > Maybe it's chipset hardcode.
> > Because our 7710 android bsp version is also ok.
> 
> This feature is chipset dependent.
> Some chipset need mute request from audioManager, some need from RIL.
> 
> 
> The information should be provided by chipset provider.
> 
> thanks!!
> sku

We need mute from RIL.
I think gecko should support more chipset, qcom and sprd, the same as android. 
You can test this case on hamachi and fugu device.

Comment 9

[shawn ku [:sku]]

The patch does not look okay to us.

1. no matter RIL or AudioManager path, it should be only one be triggered. not both of them
2. After offline discussion with RIL team, AudioManager.cpp might be the proper place to distinguish/trigger the mute/un-mute with correct path.


Hi Randy:
 Could you please give us your comment/suggestion?

Thanks!!
sku

Comment 10

[Randy Lin [:rlin] - Be Mozillian on 2015/01/26]

This patch would broke the original design, right?
I also agree sku's option because we may remove out the microphoneMuted API from telephony interface.

Comment 11

[sam.hua]

if we want distinguish/trigger the correct path in AudioManager.cpp, we will have to find out how to send the mute command to ril path.
That's the reason i distinguish it in the TelephonyProvider.js and add "setMicMute" method in ril_worker.js，the routes are ready.

Comment 12

[James Zhang (Spreadtrum)]

Can we use property just like moz.mute.ril to handle sprd chipset case.

Comment 13

[shawn ku [:sku]]

I am checking if there is way to go through mute/unmute via AudioManager.cpp.

Comment 14

[shawn ku [:sku]]

Attachment: trial patch for setMute part. (still need getMute)

you can simply change 

mRouteMuteToRIL = Preferences::GetBool("device.phone.mute_via_RIL", false);
to
mRouteMuteToRIL = Preferences::GetBool("device.phone.mute_via_RIL", true);

to make mute request through RIL, not Audio HAL.

Comment 15

[shawn ku [:sku]]

Comment on attachment 827897 [details] [diff] [review]
trial patch for setMute part. (still need getMute)

This patch for setMute is in-complete. obsolete first.

Comment 16

[shawn ku [:sku]]

Attachment: trial patch for setMute part. (still need getMute)

[Trial] Let AudioManager.cpp to handle mute request (via RIL or via HAL).
Still need getMute part.

Comment 17

[shawn ku [:sku]]

Attachment: Bug 931722 - Part1: seperate mute route at AudioManager. [Fugu]Can't mute in the call.

Comment 18

[shawn ku [:sku]]

Attachment: Bug 931722 - Part2: change signature of setMute at ril_worker. [Fugu]Can't mute in the call.

Comment 19

[Marco Chen [:mchen]]

Comment on attachment 831250 [details] [diff] [review]
Bug 931722 - Part1: seperate mute route at AudioManager. [Fugu]Can't mute in the call.

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

Hi Shawn,

Thanks for taking care this issue.

1. Is the muted state of RIL per each voice_call or not? (the ril here means components outside the gecko for telephony feature)
  ex:
    a. A first voice call is picked up then set muted to true. And hang up the call.
    b. A second voice call is picked up again.
    In this case, is the muted state in RIL "true" or "false"?
   I am afraid that muted state in RIL for second call is false but it is true in AudioManager.

2. I think we need to complete the case for multi-sim too.

::: dom/system/gonk/AudioManager.cpp
@@ +460,5 @@
>  
>  NS_IMETHODIMP
>  AudioManager::SetMicrophoneMuted(bool aMicrophoneMuted)
>  {
> +  if (mRouteMuteToRIL) {

In order to reduce the levels of if-else, we can do the "return error" before do following stuffs.
And the "return error" check can use macro to beautify the codes.

ex: 
nsCOMPtr<nsIRadioInterfaceLayer> ril = do_GetService("@mozilla.org/ril;1");
NS_ENSURE_TRUE(ril, NS_ERROR_FAILURE);

...
NS_ENSURE_TRUE(radioInterface, NS_ERROR_FAILURE);

@@ +465,5 @@
> +    nsCOMPtr<nsIRadioInterfaceLayer> ril = do_GetService("@mozilla.org/ril;1");
> +    if (ril) {
> +      nsCOMPtr<nsIRadioInterface> radioInterface;
> +      // TODO: acquire correct RadioInterface instance in Multi-SIM
> +      // configuration

Since Multi-SIM is starting to test on Fugu device, we need to make sure Multi-SIM is workable too.

@@ +470,5 @@
> +      ril->GetRadioInterface(0 /* clientId */, getter_AddRefs(radioInterface));
> +      if (radioInterface) {
> +        AutoSafeJSContext cx;
> +        JS::Rooted<JSObject*> obj(cx, JS_NewObject(cx, nullptr, nullptr,
> +                                                   nullptr));

JS::Rooted<JSObject*> obj(cx,
                          JS_NewObject(cx, nullptr, nullptr, nullptr));

Comment 20

[shawn ku [:sku]]

Hi Marco:
 Thanks for your time and comment.
Please check my inline reply, correct me if anything is wrong.

(In reply to Marco Chen [:mchen] from comment #19)
> Comment on attachment 831250 [details] [diff] [review]
> Bug 931722 - Part1: seperate mute route at AudioManager. [Fugu]Can't mute in
> the call.
> 
> Review of attachment 831250 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hi Shawn,
> 
> Thanks for taking care this issue.
> 
> 1. Is the muted state of RIL per each voice_call or not? (the ril here means
> components outside the gecko for telephony feature)
>   ex:
>     a. A first voice call is picked up then set muted to true. And hang up
> the call.
>     b. A second voice call is picked up again.
>     In this case, is the muted state in RIL "true" or "false"?
>    I am afraid that muted state in RIL for second call is false but it is
> true in AudioManager.

gaia will reset mute to false when first mt/mo. therefore, the status will be all synced from gaia<->AudioManger<->RIL<->modem.

for the case you mentioned, there should be no such un-sync probelm.
am I right?

// calls_handler.js at gaia
    // First incoming or outgoing call, reset mute and speaker.
    if (handledCalls.length == 0) {
      CallScreen.unmute();
      CallScreen.turnSpeakerOff();
    }

> 
> 2. I think we need to complete the case for multi-sim too.
> 

 Roger that.

> ::: dom/system/gonk/AudioManager.cpp
> @@ +460,5 @@
> >  
> >  NS_IMETHODIMP
> >  AudioManager::SetMicrophoneMuted(bool aMicrophoneMuted)
> >  {
> > +  if (mRouteMuteToRIL) {
> 
> In order to reduce the levels of if-else, we can do the "return error"
> before do following stuffs.
> And the "return error" check can use macro to beautify the codes.
> 
> ex: 
> nsCOMPtr<nsIRadioInterfaceLayer> ril = do_GetService("@mozilla.org/ril;1");
> NS_ENSURE_TRUE(ril, NS_ERROR_FAILURE);
> 
> ...
> NS_ENSURE_TRUE(radioInterface, NS_ERROR_FAILURE);
> 

Thanks for lecture me coding style/convention.
Will update this part in next patch.

> @@ +465,5 @@
> > +    nsCOMPtr<nsIRadioInterfaceLayer> ril = do_GetService("@mozilla.org/ril;1");
> > +    if (ril) {
> > +      nsCOMPtr<nsIRadioInterface> radioInterface;
> > +      // TODO: acquire correct RadioInterface instance in Multi-SIM
> > +      // configuration
> 
> Since Multi-SIM is starting to test on Fugu device, we need to make sure
> Multi-SIM is workable too.
>

Sure, will check how to adopt this.
 
> @@ +470,5 @@
> > +      ril->GetRadioInterface(0 /* clientId */, getter_AddRefs(radioInterface));
> > +      if (radioInterface) {
> > +        AutoSafeJSContext cx;
> > +        JS::Rooted<JSObject*> obj(cx, JS_NewObject(cx, nullptr, nullptr,
> > +                                                   nullptr));
> 
> JS::Rooted<JSObject*> obj(cx,
>                           JS_NewObject(cx, nullptr, nullptr, nullptr));

revice in next patch.


Thanks again!!
sku

Comment 21

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

Comment on attachment 831251 [details] [diff] [review]
Bug 931722 - Part2: change signature of setMute at ril_worker. [Fugu]Can't mute in the call.

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

Looks good, thanks!

Comment 22

[shawn ku [:sku]]

Attachment: Bug 931722 - Part1: seperate mute route at AudioManager. [Fugu]Can't mute in the call. v2.

Comment 23

[shawn ku [:sku]]

Attachment: Bug 931722 - Part2: change signature of setMute at ril_worker. [Fugu]Can't mute in the call. v2.

Comment 24

[shawn ku [:sku]]

Attachment: Bug 931722 - Part1: RIL related code change. [Fugu]Can't mute in the call. v2.

Comment 25

[shawn ku [:sku]]

Attachment: Bug 931722 - Part2: seperate mute route at AudioManager. [Fugu]Can't mute in the call. v2.

Comment 26

[shawn ku [:sku]]

Attachment: Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in the call.

Comment 27

[shawn ku [:sku]]

Hi Etienne:
 We plan to "reset mute state to false" when user try to dial/answer calls at TelephonyProvider.
Could you please have a look to see if any impact regarding Gaia? Or anything need to be synced between Gaia and Gecko?

Thanks!!
sku

Comment 28

[Etienne Segonzac (:etienne)]

I think we're good.
We're already doing it in gaia I think :)
https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/calls_handler.js#L174-178

Comment 29

[Marco Chen [:mchen]]

Comment on attachment 832841 [details] [diff] [review]
Bug 931722 - Part2: seperate mute route at AudioManager. [Fugu]Can't mute in the call. v2.

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

In this patch, we did a lot of things related to RIL module.
Maybe keep these stuffs into RIL module itself and expose an interface for AudioManager to trigger mute/unmute is more clear between RIL and AudioManager.

::: dom/system/gonk/AudioManager.cpp
@@ +454,4 @@
>    }
> +
> +  NS_ENSURE_FALSE(AudioSystem::isMicrophoneMuted(aMicrophoneMuted),
> +                  NS_ERROR_FAILURE);

This is my personal opinion.
I feel use macro here is too over-engineering already.

Comment 30

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

Comment on attachment 832840 [details] [diff] [review]
Bug 931722 - Part1: RIL related code change. [Fugu]Can't mute in the call. v2.

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

Really sorry for the delay. :(

Per offline discussion with Shawn and Marco, we agree that having a new attribute exposing to nsIRadioInterfaceLayer is better for clear modulization. Please revise the patch for that. Thank you!

Comment 31

[shawn ku [:sku]]

Attachment: Bug 931722 - Part1: RIL related code change. [Fugu]Can't mute in the call. v3.

Comment 32

[shawn ku [:sku]]

Attachment: Bug 931722 - Part2: seperate mute route at AudioManager. [Fugu]Can't mute in the call. v3.

Comment 33

[shawn ku [:sku]]

Attachment: Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in the call. v3.

Comment 34

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

Comment on attachment 8338239 [details] [diff] [review]
Bug 931722 - Part1: RIL related code change. [Fugu]Can't mute in the call. v3.

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

::: dom/telephony/gonk/TelephonyProvider.js
@@ +390,5 @@
>        aNumber = gPhoneNumberUtils.normalize(aNumber);
>      }
>      if (this._validateNumber(aNumber)) {
> +      // set mute to false before dialing request.
> +      this.microphoneMuted = false;

Per comment 28, I don't see why we need this still.

@@ +412,5 @@
>    },
>  
>    answerCall: function(aClientId, aCallIndex) {
> +    // set mute to false before answering a call.
> +    this.microphoneMuted = false;

ditto.

Comment 35

[shawn ku [:sku]]

Hi Hsin-Yi:
 Thanks for your comment.

I would like to clarify one thing here.

By reading the code that Etienne provided, we can see gaia only reset mute state to false then "first" MT/MO call.
My consideration here (refer to AOSP behaviour) is we might need to reset mute to false when every dial/answer request.
For example, 1 Active + 1 Waiting with muted state, user try to answer the wating call, we unmute the call automatically for user's convenient. if we don't do so, device still keep in muted state after answering waiting call.

Please correct me if anything is wrong in your viewpoint. or give me some extra suggestion.

Thanks!!
sku





(https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/js/calls_handler.js#L174-178)
    // First incoming or outgoing call, reset mute and speaker.
    if (handledCalls.length == 0) {
      CallScreen.unmute();
      CallScreen.turnSpeakerOff();
    }

Comment 36

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

(In reply to shawn ku [:sku] from comment #35)
> Hi Hsin-Yi:
>  Thanks for your comment.
> 
> I would like to clarify one thing here.
> 
> By reading the code that Etienne provided, we can see gaia only reset mute
> state to false then "first" MT/MO call.
> My consideration here (refer to AOSP behaviour) is we might need to reset
> mute to false when every dial/answer request.
> For example, 1 Active + 1 Waiting with muted state, user try to answer the
> wating call, we unmute the call automatically for user's convenient. if we
> don't do so, device still keep in muted state after answering waiting call.
> 
Thanks for the explanation, Shawn :)

I am fine with this user behaviour. However I think this is out of scope of this bug. I'd suggest filing another bug to have thorough discussion on mute behaviour. For example, if user mutes before a call is putting on hold, then should we unmute it automatically when the call is resumed? Another concern is, even we want to mute/unmute automatically for user's convenience, where should we handle this, gaia or gecko? Looks gaia is taking part of it, isn't it better to let gaia manage the whole logic instead of spreading the same thing into different components? 

> Please correct me if anything is wrong in your viewpoint. or give me some
> extra suggestion.
> 
> Thanks!!
> sku
> 
> 
> 
> 
> 
> (https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/dialer/
> js/calls_handler.js#L174-178)
>     // First incoming or outgoing call, reset mute and speaker.
>     if (handledCalls.length == 0) {
>       CallScreen.unmute();
>       CallScreen.turnSpeakerOff();
>     }

Comment 37

[shawn ku [:sku]]

Attachment: Bug 931722 - Part1: RIL related code change. [Fugu]Can't mute in the call. v4.

Comment 38

[shawn ku [:sku]]

Attachment: Bug 931722 - Part2: seperate mute route at AudioManager. [Fugu]Can't mute in the call. v4.

Comment 39

[shawn ku [:sku]]

Attachment: Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in the call. v4.

Comment 40

[shawn ku [:sku]]

Attachment: Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in the call. v4.

Comment 41

[shawn ku [:sku]]

Attachment: Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in the call. v4.

Comment 42

[shawn ku [:sku]]

Comment on attachment 8338312 [details] [diff] [review]
Bug 931722 - Part1: RIL related code change. [Fugu]Can't mute in the call. v4.

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

remove reset mute at TelephonyProvider.js

Comment 43

[shawn ku [:sku]]

Comment on attachment 8338313 [details] [diff] [review]
Bug 931722 - Part2: seperate mute route at AudioManager. [Fugu]Can't mute in the call. v4.

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

change reading preference to system property to determine which mute path should go.

Comment 44

[shawn ku [:sku]]

Comment on attachment 8338336 [details] [diff] [review]
Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in the call. v4.

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

Add test cases to make sure set/get mute via telephony work as expect.

Comment 45

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

Comment on attachment 8338312 [details] [diff] [review]
Bug 931722 - Part1: RIL related code change. [Fugu]Can't mute in the call. v4.

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

Thanks! r=me with the comments addressed.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +599,5 @@
> +
> +  setMicrophoneMuted: function setMicrophoneMuted(muted) {
> +    for (let clientId = 0; clientId < this.numRadioInterfaces; clientId++) {
> +      let radioInterface = this.radioInterfaces[clientId];
> +      radioInterface.workerMessenger.send("setMute", { mute: muted });

nit: s/mute/muted/

::: dom/system/gonk/ril_worker.js
@@ +1454,3 @@
>      Buf.newParcel(REQUEST_SET_MUTE);
>      Buf.writeInt32(1);
> +    Buf.writeInt32(options.mute ? 1 : 0);

s/mute/muted/

Comment 46

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

Comment on attachment 8338336 [details] [diff] [review]
Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in the call. v4.

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

General comment:

I don't see the connection b/w mute attribute and call states.
If test_call_mute.js is to test if telephony.muted works as expected, why not simply do the following:
    telephony.muted = false;
    is(telephony.muted, false);
    telephony.muted = true;
    is(telephony.muted, true);

::: dom/telephony/test/marionette/test_call_mute.js
@@ +3,5 @@
> +
> +MARIONETTE_TIMEOUT = 60000;
> +MARIONETTE_HEAD_JS = 'head.js';
> +
> +let telephony = window.navigator.mozTelephony;

We've done this in 'head.js.' Remove it.

@@ +9,5 @@
> +let outgoingCall;
> +let number = "1234567890";
> +
> +startTest(function() {
> +  test_outgoing_call();

Please move this function to the end of the file. We follow the convention in telephony marionette tests.

@@ +50,5 @@
> +      incomingCall.hangUp();
> +    };
> +
> +    incomingCall.ondisconnected = function ondisconnected(event) {
> +      cleanUp();

Make sure there's no calls in emulator ('gsm list') before cleanUp().

Please add this:
emulator.run("gsm list", function(result) {
      log("Call list is now: " + result);
      is(result[0], "OK");
      cleanUp();
    });

Comment 47

[shawn ku [:sku]]

Attachment: Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in the call. v5.

Comment 48

[shawn ku [:sku]]

Comment on attachment 8338388 [details] [diff] [review]
Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in the call. v5.

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

Thanks HsinYi,
It is my bad to keep call state with mute.
My original purpose was to make sure we got un-muted state with every dial/answer request. however, this will be move to new bug for discussion first.

I should only keep below part just to make sure APIs works as expected.

+  telephony.muted = true;
+  is(telephony.muted, true);
+  telephony.muted = false;
+  is(telephony.muted, false);

Thanks again!!

Comment 49

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

Comment on attachment 8338388 [details] [diff] [review]
Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in the call. v5.

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

Thank you!

Comment 50

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

(In reply to shawn ku [:sku] from comment #48)
> Comment on attachment 8338388 [details] [diff] [review]
> Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in
> the call. v5.
> 
> Review of attachment 8338388 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks HsinYi,
> It is my bad to keep call state with mute.

No worries :)

> My original purpose was to make sure we got un-muted state with every
> dial/answer request. however, this will be move to new bug for discussion
> first.
> 
> I should only keep below part just to make sure APIs works as expected.
> 
> +  telephony.muted = true;
> +  is(telephony.muted, true);
> +  telephony.muted = false;
> +  is(telephony.muted, false);
> 
> Thanks again!!

Comment 51

[Marco Chen [:mchen]]

Comment on attachment 8338313 [details] [diff] [review]
Bug 931722 - Part2: seperate mute route at AudioManager. [Fugu]Can't mute in the call. v4.

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

::: dom/system/gonk/AudioManager.cpp
@@ +423,5 @@
>      NS_WARNING("Failed to add mozsettings-changed observer!");
>    }
> +
> +  char value[PROPERTY_VALUE_MAX];
> +  property_get("device.send-mic-mute-to-ril", value, "0");

1. Refer to the old property for FxOS, the naming will be "ro.moz.XXX".
ex: ro.moz.audio.mute.from_ril

2. And I suggest to set value as "true" or "false" and that will be more readable.

@@ +472,5 @@
> +    NS_ENSURE_TRUE(ril, NS_ERROR_FAILURE);
> +    ril->SetMicrophoneMuted(aMicrophoneMuted);
> +    mIsMicMuted = aMicrophoneMuted;
> +    return NS_OK;
> +  }

I am sorry to bring this question late.

If the mute is came from audio recording app and video conference app in the future (not in voice_call state.)
Is sending mute to ril really workable?
Or we need to enable mute from RIL and AudioSystem.

Comment 52

[James Zhang (Spreadtrum)]

When can I add "device.send-mic-mute-to-ril=1" property to fugu device?

Comment 53

[James Zhang (Spreadtrum)]

(In reply to Marco Chen [:mchen] from comment #51)
> Comment on attachment 8338313 [details] [diff] [review]
> Bug 931722 - Part2: seperate mute route at AudioManager. [Fugu]Can't mute in
> the call. v4.
> 
> Review of attachment 8338313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/AudioManager.cpp
> @@ +423,5 @@
> >      NS_WARNING("Failed to add mozsettings-changed observer!");
> >    }
> > +
> > +  char value[PROPERTY_VALUE_MAX];
> > +  property_get("device.send-mic-mute-to-ril", value, "0");
> 
> 1. Refer to the old property for FxOS, the naming will be "ro.moz.XXX".
> ex: ro.moz.audio.mute.from_ril
> 
> 2. And I suggest to set value as "true" or "false" and that will be more
> readable.
> 
> @@ +472,5 @@
> > +    NS_ENSURE_TRUE(ril, NS_ERROR_FAILURE);
> > +    ril->SetMicrophoneMuted(aMicrophoneMuted);
> > +    mIsMicMuted = aMicrophoneMuted;
> > +    return NS_OK;
> > +  }
> 
> I am sorry to bring this question late.
> 
> If the mute is came from audio recording app and video conference app in the
> future (not in voice_call state.)
> Is sending mute to ril really workable?
> Or we need to enable mute from RIL and AudioSystem.

Marco, you're right, ro.moz.audio.mute.from_ril=true or false is better name.

Comment 54

[shawn ku [:sku]]

Hi Marco:
 I will update the naming for property today, thanks for addressing it.

Hi James:
 Could you please help check/answer question from Marco about comment 51?

Comment 55

[James Zhang (Spreadtrum)]

(In reply to Marco Chen [:mchen] from comment #51)
> Comment on attachment 8338313 [details] [diff] [review]
> Bug 931722 - Part2: seperate mute route at AudioManager. [Fugu]Can't mute in
> the call. v4.
> 
> Review of attachment 8338313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/AudioManager.cpp
> @@ +423,5 @@
> >      NS_WARNING("Failed to add mozsettings-changed observer!");
> >    }
> > +
> > +  char value[PROPERTY_VALUE_MAX];
> > +  property_get("device.send-mic-mute-to-ril", value, "0");
> 
> 1. Refer to the old property for FxOS, the naming will be "ro.moz.XXX".
> ex: ro.moz.audio.mute.from_ril
> 
> 2. And I suggest to set value as "true" or "false" and that will be more
> readable.
> 
> @@ +472,5 @@
> > +    NS_ENSURE_TRUE(ril, NS_ERROR_FAILURE);
> > +    ril->SetMicrophoneMuted(aMicrophoneMuted);
> > +    mIsMicMuted = aMicrophoneMuted;
> > +    return NS_OK;
> > +  }
> 

It's ok.

> I am sorry to bring this question late.
> 
> If the mute is came from audio recording app and video conference app in the
> future (not in voice_call state.)
> Is sending mute to ril really workable?
> Or we need to enable mute from RIL and AudioSystem.

Sam will check and answer your question.

Comment 56

[shawn ku [:sku]]

Attachment: 8338313: Bug 931722 - Part2: seperate mute route at AudioManager. [Fugu]Can't mute in the call. v5.

Comment 57

[shawn ku [:sku]]

Attachment: Bug 931722 - Part2: seperate mute route at AudioManager. [Fugu]Can't mute in the call. v5.

Comment 58

[shawn ku [:sku]]

Attachment: Bug 931722 - Part2: extra mute request to RIL at AudioManager. [Fugu]Can't mute in the call. v5.

Comment 59

[Marco Chen [:mchen]]

Comment on attachment 8339029 [details] [diff] [review]
Bug 931722 - Part2: extra mute request to RIL at AudioManager. [Fugu]Can't mute in the call. v5.

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

It looks good for me now. r=me.

And please wait for confirmation from partner then asking for landing.
Thanks.

Comment 60

[shawn ku [:sku]]

Attachment: Bug 931722 - Part1: RIL related code change. [Fugu]Can't mute in the call. r=hsinyi.

Comment 61

[shawn ku [:sku]]

Attachment: Bug 931722 - Part2: extra mute request to RIL at AudioManager. [Fugu]Can't mute in the call. r=mchen.

Comment 62

[shawn ku [:sku]]

Attachment: 931722.test.4.patch

Comment 63

[shawn ku [:sku]]

Attachment: Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in the call. r=hsinyi.

Comment 64

[shawn ku [:sku]]

try result:
https://tbpl.mozilla.org/?tree=Try&rev=9dc6cad41b2e

Comment 65

[sam.hua]

(In reply to James Zhang from comment #55)
> (In reply to Marco Chen [:mchen] from comment #51)
> > If the mute is came from audio recording app and video conference app in the
> > future (not in voice_call state.)
> > Is sending mute to ril really workable?
> > Or we need to enable mute from RIL and AudioSystem.
> 
> Sam will check and answer your question.

1. audio recording app don't have mute function now. and, it is controlled by app that send mute to Audio or RIL.

2. Video call should use RIL

Comment 66

[Steven Yang [:styang]]

gonna check in to v1.2, any concern?

Comment 67

[shawn ku [:sku]]

Hi Sam:
 Please setup below property to make mute work on fugu.
(We are still waiting for code phase-in, should be ready by next week)

ro.moz.mute.call.to_ril=true

Thanks!!
sku

Comment 68

[James Zhang (Spreadtrum)]

(In reply to shawn ku [:sku] from comment #67)
> Hi Sam:
>  Please setup below property to make mute work on fugu.
> (We are still waiting for code phase-in, should be ready by next week)
> 
> ro.moz.mute.call.to_ril=true
> 
> Thanks!!
> sku

OK

Comment 69

[James Zhang (Spreadtrum)]

(In reply to shawn ku [:sku] from comment #67)
> Hi Sam:
>  Please setup below property to make mute work on fugu.
> (We are still waiting for code phase-in, should be ready by next week)
> 
> ro.moz.mute.call.to_ril=true
> 
> Thanks!!
> sku

Done.

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/0815ba262621
https://hg.mozilla.org/integration/b2g-inbound/rev/3117fa37236a
https://hg.mozilla.org/integration/b2g-inbound/rev/10de0a51d2ba

Comment 71

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0815ba262621
https://hg.mozilla.org/mozilla-central/rev/3117fa37236a
https://hg.mozilla.org/mozilla-central/rev/10de0a51d2ba

Comment 72

[Ryan VanderMeulen [:RyanVM]]

This is going to need branch-specific patches for the b2g26 uplift.

Comment 73

[[:fabrice] Fabrice Desré]

(In reply to Steven Yang [:styang] from comment #66)
> gonna check in to v1.2, any concern?

I'm only sherrifing 1.3. You need to ask approval for 1.2 on the patch itself.

Comment 74

[James Ho]

This one should go to 1.2f.

Comment 75

[Keven Kuo [:kkuo]]

It should go to 1.3 also. Then it will be of benefit to new device.

--
Keven

Comment 76

[shawn ku [:sku]]

Hi Ryan:
 As James/Keven said, this patch should go to m-c(including 1.3)/1.2f.

Please let me know if anything I missed.
Thanks!!
sku

Comment 77

[Ryan VanderMeulen [:RyanVM]]

It's still going to need branch-specific patches for 1.2f of b2g26. It's already in 1.3 (m-c).

Comment 78

[shawn ku [:sku]]

Thanks Ryan, I will prepare a patch and upload to bugzilla tomorrow (due to I am OOO today).

Comment 79

[shawn ku [:sku]]

Attachment: [Patch for 1.2f] Bug 931722 - Part1: RIL related code change. [Fugu]Can't mute in the call. r=hsinyi. a=fugu+.

Comment 80

[shawn ku [:sku]]

Attachment: [Patch for 1.2f] Bug 931722 - Part2: extra mute request to RIL at AudioManager. [Fugu]Can't mute in the call. r=mchen. a=fugu+.

Comment 81

[shawn ku [:sku]]

Attachment: 931722.1.2f.test.patch

Comment 82

[shawn ku [:sku]]

Attachment: [Patch for 1.2f] Bug 931722 - Part3: Add test cases for call mute test. [Fugu]Can't mute in the call. r=hsinyi. a=fugu+.

Comment 83

[shawn ku [:sku]]

Try result -
https://tbpl.mozilla.org/?tree=Try&rev=e6a9a0812f52

Hi Ryan:
 I merge the patch based on 1.2f repo.
Please help me check if anything I missed.

Thanks!!
shawn

Comment 84

[Ryan VanderMeulen [:RyanVM]]

This doesn't have fugu+ blocking status yet. Once it does and the status-b2g-v1.2f flag is created, I'll uplift it. I'll point out that koi+ status (which it had at one point) would have gotten it on to v1.2 (which then merges to v1.2f automatically).

Comment 85

[Keven Kuo [:kkuo]]

Hi! Steven,

Please set fugu+. Thanks.

--
Keven

Comment 86

[sam.hua]

Hi shawn,
anything should i do for this bug?

Comment 87

[shawn ku [:sku]]

Hi Sam:
 If three "[Patch for 1.2f]..." are all merged into 1.2f, there is nothing to do with this bug.
Thanks!!
sku