Add an option to disable built-in AEC to AppRTC Android Demo

Add an option to disable built-in AEC to AppRTC Android Demo

BUG=webrtc:5923
Committed: https://crrev.com/c00687ff5d01670173f3a2466495986d65461dc7
Cr-Commit-Position: refs/heads/master@{#12885}

[sakal, 2016-05-23 14:30:28]

Description was changed from

==========
Add an option to disable built-in AEC to AppRTC Android Demo

BUG=webrtc:5923
==========

to

==========
Add an option to disable built-in AEC to AppRTC Android Demo

BUG=webrtc:5923
==========

[sakal, 2016-05-23 14:30:28]

sakal@webrtc.org changed reviewers:
+ henrika@webrtc.org, magjed@webrtc.org

[sakal, 2016-05-23 14:33:45]

Magnus and Henrik, can you take a look please?

Henrik, I added public methods to WebRtcAudioUtils to expose WebRtcAudioEffects
methods. I copied those methods from WebRtcAudioManager where they are private
methods. I could have also made those ones public. However, WebRtcAudioUtils
feels like a more logical place for them.

[henrika_webrtc, 2016-05-23 14:59:02]

Looks good.

I think you must verify here:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
that your setting really has the correct effect as well.

[henrika_webrtc, 2016-05-23 14:59:19]

https://codereview.webrtc.org/2002093002/diff/20001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java
(right):

https://codereview.webrtc.org/2002093002/diff/20001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java:112: //
Disable forcing WebRTC based AEC so it won't our value
The comment sounds strange. Is it a type? Should it be "so it won't use our
value"?
Please add some more details around this part.

[magjed_webrtc, 2016-05-24 08:31:18]

https://codereview.webrtc.org/2002093002/diff/40001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java
(right):

https://codereview.webrtc.org/2002093002/diff/40001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:298: 
nit: One empty line is enough.

https://codereview.webrtc.org/2002093002/diff/40001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java
(right):

https://codereview.webrtc.org/2002093002/diff/40001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java:115:
if(!WebRtcAudioUtils.isAcousticEchoCancelerSupported()) {
nit: space between 'if' and '('.

https://codereview.webrtc.org/2002093002/diff/40001/webrtc/examples/androidte...
File
webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java
(right):

https://codereview.webrtc.org/2002093002/diff/40001/webrtc/examples/androidte...
webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java:265:
new PeerConnectionParameters(
Not really related to your CL, but since you touch this, please comment each
literal parameter, like
new PeerConnectionParameters((
    false, /* videoCallEnabled */,
    true, /* loopback */,
    false, /* tracing */
    ...

[sakal, 2016-05-24 10:54:29]

On 2016/05/23 14:59:02, henrika_webrtc wrote:
> Looks good.
> 
> I think you must verify here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> that your setting really has the correct effect as well.

[sakal, 2016-05-24 10:56:36]

https://codereview.webrtc.org/2002093002/diff/20001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java
(right):

https://codereview.webrtc.org/2002093002/diff/20001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java:112: //
Disable forcing WebRTC based AEC so it won't our value
On 2016/05/23 14:59:19, henrika_webrtc wrote:
> The comment sounds strange. Is it a type? Should it be "so it won't use our
> value"?
> Please add some more details around this part.

Oops, typo.

https://codereview.webrtc.org/2002093002/diff/40001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java
(right):

https://codereview.webrtc.org/2002093002/diff/40001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/ConnectActivity.java:298: 
On 2016/05/24 08:31:18, magjed_webrtc wrote:
> nit: One empty line is enough.

Done.

https://codereview.webrtc.org/2002093002/diff/40001/webrtc/examples/androidap...
File webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java
(right):

https://codereview.webrtc.org/2002093002/diff/40001/webrtc/examples/androidap...
webrtc/examples/androidapp/src/org/appspot/apprtc/SettingsActivity.java:115:
if(!WebRtcAudioUtils.isAcousticEchoCancelerSupported()) {
On 2016/05/24 08:31:18, magjed_webrtc wrote:
> nit: space between 'if' and '('.

Done.

https://codereview.webrtc.org/2002093002/diff/40001/webrtc/examples/androidte...
File
webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java
(right):

https://codereview.webrtc.org/2002093002/diff/40001/webrtc/examples/androidte...
webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java:265:
new PeerConnectionParameters(
On 2016/05/24 08:31:18, magjed_webrtc wrote:
> Not really related to your CL, but since you touch this, please comment each
> literal parameter, like
> new PeerConnectionParameters((
>     false, /* videoCallEnabled */,
>     true, /* loopback */,
>     false, /* tracing */
>     ...

Done.

[sakal, 2016-05-24 10:57:22]

On 2016/05/23 14:59:02, henrika_webrtc wrote:
> Looks good.
> 
> I think you must verify here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/webrtc...
> that your setting really has the correct effect as well.

Verified, fixed bug in Patch Set #5.

[henrika_webrtc, 2016-05-24 11:14:00]

LGTM. Nice work ;-)

https://codereview.webrtc.org/2002093002/diff/100001/webrtc/examples/androida...
File webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java
(right):

https://codereview.webrtc.org/2002093002/diff/100001/webrtc/examples/androida...
webrtc/examples/androidapp/src/org/appspot/apprtc/PeerConnectionClient.java:350:
Log.d(TAG, "Allow built-in AEC if device supports it");
Comment is a bit vague. Is it enabled or disabled? If more parts are involved,
add comment about it perhaps.

[magjed_webrtc, 2016-05-24 14:16:21]

lgtm

https://codereview.webrtc.org/2002093002/diff/120001/webrtc/examples/androidt...
File
webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java
(right):

https://codereview.webrtc.org/2002093002/diff/120001/webrtc/examples/androidt...
webrtc/examples/androidtests/src/org/appspot/apprtc/test/PeerConnectionClientTest.java:311:

nit: remove

[sakal, 2016-05-24 14:18:46]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2016-05-24 14:18:47]

The patchset sent to the CQ was uploaded after l-g-t-m from henrika@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2002093002/#ps120001 (title:
"Fixes according to henrika's comments")

[commit-bot: I haz the power, 2016-05-24 14:18:51]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2002093002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2002093002/120001

[sakal, 2016-05-24 14:18:58]

The CQ bit was unchecked by sakal@webrtc.org

[sakal, 2016-05-25 07:03:41]

The CQ bit was checked by sakal@webrtc.org

[sakal, 2016-05-25 07:03:42]

The patchset sent to the CQ was uploaded after l-g-t-m from magjed@webrtc.org,
henrika@webrtc.org
Link to the patchset: https://codereview.webrtc.org/2002093002/#ps140001 (title:
"Remove empty line")

[commit-bot: I haz the power, 2016-05-25 07:03:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2002093002/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2002093002/140001

[commit-bot: I haz the power, 2016-05-25 07:09:50]

Description was changed from

==========
Add an option to disable built-in AEC to AppRTC Android Demo

BUG=webrtc:5923
==========

to

==========
Add an option to disable built-in AEC to AppRTC Android Demo

BUG=webrtc:5923
==========

[commit-bot: I haz the power, 2016-05-25 07:09:51]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-05-25 07:09:58]

Description was changed from

==========
Add an option to disable built-in AEC to AppRTC Android Demo

BUG=webrtc:5923
==========

to

==========
Add an option to disable built-in AEC to AppRTC Android Demo

BUG=webrtc:5923

Committed: https://crrev.com/c00687ff5d01670173f3a2466495986d65461dc7
Cr-Commit-Position: refs/heads/master@{#12885}
==========

[commit-bot: I haz the power, 2016-05-25 07:09:59]

Patchset 8 (id:??) landed as
https://crrev.com/c00687ff5d01670173f3a2466495986d65461dc7
Cr-Commit-Position: refs/heads/master@{#12885}