Improving support for Android Audio Effects in WebRTC

Improving support for Android Audio Effects in WebRTC.
Now also supports AGC and NS effects and adds the possibility
to override default settings.

R=magjed@webrtc.org, pbos@webrtc.org, sophiechang@chromium.org
TBR=perkj
BUG=NONE

Committed: https://crrev.com/c14f5ff60fb0c42c97702de112a9e8f1eccba574
Cr-Commit-Position: refs/heads/master@{#10030}

[henrika_webrtc, 2015-09-21 16:02:00]

henrika@webrtc.org changed reviewers:
+ magjed@webrtc.org, sophiechang@chromium.org

[henrika_webrtc, 2015-09-21 16:05:56]

magjed@ for all parts and
sophiechang@ for parts under android/java/src/org/webrtc/voiceengine/

[henrika_webrtc, 2015-09-21 16:13:12]

FYI, I still have some more work todo in Libjingle but the main parts should be
possible to review as is.

[sophiechang-webrtc, 2015-09-21 20:04:40]

Definitely much better than previous approach and more flexible from an API
standpoint.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:43:
private static Boolean canUseAcousticEchoCanceler = null;
i'm really not a fan of tri-state booleans - if the values are getting cached
anyway, can't we just set these to false?

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:206:
// Four conditions must be fulfilled if true shall be returned:
"if true shall be returned" -> "if functions are to return true"

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java:33:
// the low latency ouput mode in combination with OpenSL ES.
ouput -> output

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java:98:
public static boolean defaultSampleRateIsOverriden() {
is there a basic java style guide that is being followed here? the methods in
WebRtcAudioManager have the first letter capitalized (which i haven't seen in
any java)? 

i would probably recommend something like 
public static boolean isDefaultSampleRateOverridden() since all the other
methods in this file start with verbs.

[henrika_webrtc, 2015-09-22 11:57:04]

Thanks Sophie ;-)

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:43:
private static Boolean canUseAcousticEchoCanceler = null;
I am not using them as tri-state here IMHO. The current design ensures that
I don't do expensive work more than once even if the result is false.
I would need an extra state otherwise to keep track of is the member is defined
or not.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:206:
// Four conditions must be fulfilled if true shall be returned:
On 2015/09/21 20:04:40, sophiechang-webrtc wrote:
> "if true shall be returned" -> "if functions are to return true"

Done.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java:33:
// the low latency ouput mode in combination with OpenSL ES.
On 2015/09/21 20:04:40, sophiechang-webrtc wrote:
> ouput -> output

Done.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java:98:
public static boolean defaultSampleRateIsOverriden() {
Fixed. Not sure which methods you mean here. Can't find any where the first
letter is capitalized.

[magjed_webrtc, 2015-09-22 12:10:10]

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:43:
private static Boolean canUseAcousticEchoCanceler = null;
Can you add a comment that null means that the Booleans are not evaluated yet?

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:65:
if (!WebRtcAudioUtils.runningOnJellyBeanOrHigher()) {
If you don't intend to log anything in this if-statement, you can simplify to:
return WebRtcAudioUtils.runningOnJellyBeanOrHigher() &&
AcousticEchoCanceler.isAvailable();

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:117:
if (d.type.equals(AudioEffect.EFFECT_TYPE_AEC))
Single statement 'if' should still have braces in Java.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:118:
excluded = d.uuid.equals(AOSP_ACOUSTIC_ECHO_CANCELER);
What happens if you first find an AOSP_ACOUSTIC_ECHO_CANCELER, i.e. set
|excluded| to true, and then iterate to another EFFECT_TYPE_AEC that is not
AOSP_ACOUSTIC_ECHO_CANCELER and override |excluded| to false? Is that the
intended behaviour? Otherwise, I would write something like:
for (...) {
  if (d.type.equals(AudioEffect.EFFECT_TYPE_AEC) &&
d.uuid.equals(AOSP_ACOUSTIC_ECHO_CANCELER) {
    return true;
  }
}
return false;

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:149:
boolean isAvailable =
I would remove |isAvailable| and assign directly, i.e.
'canUseAcousticEchoCanceler = ' or 'canUseAcousticEchoCanceler = new Boolean('
here.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:156:
Logging.d(TAG, "canUseAcousticEchoCanceler: " + canUseAcousticEchoCanceler);
I would move this Loggin inside the if-statement so we only print it once.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:202:
for (Descriptor d : descriptors) {
Remove |descriptors| and inline AudioEffect.queryEffects() in the for-each loop.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:215:
// of the AEC effect is modified.
Add a comment what this function returns. Success?

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:219:
if (!canUseAcousticEchoCanceler()) {
Shouldn't this be 'if (enable && !canUseAcousticEchoCanceler()) {'?

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:221:
return false;
Can you set |shouldEnableAec| to false before returning? Even if it's redundant
I think you should add it for clarity.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:257:
assertTrue((aec == null) && (agc == null) && (ns == null));
I would remove the unnecessary parentheses inside this check. Also consider
splitting it up in three assertTrue() so we know immediately from the stack
trace which one failed.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:271:
", enable: " + (enable ? "true" : "false") +
Isn't '... + (enable ? "true" : "false")' exactly the same thing as '... +
enable' in Java? I.e. when you print a boolean it will do exactly what you do
manually here?

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:343:
return type.equals(AudioEffect.EFFECT_TYPE_AEC)
You can also consider writing it like this:
switch (type) {
  case EFFECT_TYPE_AEC:
  case EFFECT_TYPE_AGC:
  case EFFECT_TYPE_NS:
    return true;
  default:
    return false;
}

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:179:
int sampleRateHz = 0;
s/int sampleRateHz = 0/final int sampleRateHz/g.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:123:
if (effects == null) {
if (enable && effects == null) instead?

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java:61:
public static void setWebRtcBasedAcousticEchoCanceler(boolean enable) {
What about thread safety in these static functions? Why is it not necessary to
synchronize?

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java:93:
public static void setDefaultSampleRateHz(int sampleRateHz) {
ditto - Thread safety?

[henrika_webrtc, 2015-09-22 12:10:47]

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java:98:
public static boolean defaultSampleRateIsOverriden() {
But, we do have an odd style in WebRtcAudioRecord and WebRtcAudioTrack. Let me
fix that in this CL as well.

[magjed_webrtc, 2015-09-22 14:19:05]

Java files lgtm % nits.
One general comment is that it would be nice if we could send the parameters
defaultSampleRateHz, useWebRtcBasedAcousticEchoCanceler,
useWebRtcBasedAutomaticGainControl, and useWebRtcBasedNoiseSuppressor as
arguments to some constructor/init function instead of passing them via global
state, but I don't know this area of the code well enough to understand the pros
and cons.

https://codereview.webrtc.org/1344563002/diff/160001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java
(right):

https://codereview.webrtc.org/1344563002/diff/160001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:66:
return WebRtcAudioUtils.runningOnJellyBeanOrHigher() &&
&& should be on the next line

https://codereview.webrtc.org/1344563002/diff/160001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:211:
// of the AEC effect is modified. Returns true if HW AEC is supported and
So the return value is exactly like canUseAcousticEchoCanceler(). Who needs
this? I think it is strange that setAEC(false) logs stuff about AEC not being
supported when we want to disable it.

[henrika_webrtc, 2015-09-22 14:52:58]

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:43:
private static Boolean canUseAcousticEchoCanceler = null;
On 2015/09/22 12:10:09, magjed_webrtc wrote:
> Can you add a comment that null means that the Booleans are not evaluated yet?

Done.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:65:
if (!WebRtcAudioUtils.runningOnJellyBeanOrHigher()) {
On 2015/09/22 12:10:09, magjed_webrtc wrote:
> If you don't intend to log anything in this if-statement, you can simplify to:
> return WebRtcAudioUtils.runningOnJellyBeanOrHigher() &&
> AcousticEchoCanceler.isAvailable();

Done.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:117:
if (d.type.equals(AudioEffect.EFFECT_TYPE_AEC))
On 2015/09/22 12:10:09, magjed_webrtc wrote:
> Single statement 'if' should still have braces in Java.

Done.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:118:
excluded = d.uuid.equals(AOSP_ACOUSTIC_ECHO_CANCELER);
Only one effect type EFFECT_TYPE_AEC can exist.
Will change anyhow. Thanks.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:118:
excluded = d.uuid.equals(AOSP_ACOUSTIC_ECHO_CANCELER);
On 2015/09/22 12:10:10, magjed_webrtc wrote:
> What happens if you first find an AOSP_ACOUSTIC_ECHO_CANCELER, i.e. set
> |excluded| to true, and then iterate to another EFFECT_TYPE_AEC that is not
> AOSP_ACOUSTIC_ECHO_CANCELER and override |excluded| to false? Is that the
> intended behaviour? Otherwise, I would write something like:
> for (...) {
>   if (d.type.equals(AudioEffect.EFFECT_TYPE_AEC) &&
> d.uuid.equals(AOSP_ACOUSTIC_ECHO_CANCELER) {
>     return true;
>   }
> }
> return false;

Done.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:149:
boolean isAvailable =
On 2015/09/22 12:10:09, magjed_webrtc wrote:
> I would remove |isAvailable| and assign directly, i.e.
> 'canUseAcousticEchoCanceler = ' or 'canUseAcousticEchoCanceler = new Boolean('
> here.

Done.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:156:
Logging.d(TAG, "canUseAcousticEchoCanceler: " + canUseAcousticEchoCanceler);
On 2015/09/22 12:10:10, magjed_webrtc wrote:
> I would move this Loggin inside the if-statement so we only print it once.

Done.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:202:
for (Descriptor d : descriptors) {
On 2015/09/22 12:10:09, magjed_webrtc wrote:
> Remove |descriptors| and inline AudioEffect.queryEffects() in the for-each
loop.

Done.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:215:
// of the AEC effect is modified.
Improved comments.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:219:
if (!canUseAcousticEchoCanceler()) {
I don't think so since I want to ensure that we don't try to
set the state (to any state) if the effect is not supported at all.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:221:
return false;
On 2015/09/22 12:10:10, magjed_webrtc wrote:
> Can you set |shouldEnableAec| to false before returning? Even if it's
redundant
> I think you should add it for clarity.

Done.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:257:
assertTrue((aec == null) && (agc == null) && (ns == null));
On 2015/09/22 12:10:09, magjed_webrtc wrote:
> I would remove the unnecessary parentheses inside this check. Also consider
> splitting it up in three assertTrue() so we know immediately from the stack
> trace which one failed.

Done.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:271:
", enable: " + (enable ? "true" : "false") +
Yes ;-)

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:343:
return type.equals(AudioEffect.EFFECT_TYPE_AEC)
Not sure actually. Don't we need type.equals()?

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioManager.java:179:
int sampleRateHz = 0;
On 2015/09/22 12:10:10, magjed_webrtc wrote:
> s/int sampleRateHz = 0/final int sampleRateHz/g.

Done.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioRecord.java:123:
if (effects == null) {
Even if enable is false, I still have to check effects.
Don't see how I can rewrite.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java:61:
public static void setWebRtcBasedAcousticEchoCanceler(boolean enable) {
Thanks. Adding synchronized methods.

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioUtils.java:93:
public static void setDefaultSampleRateHz(int sampleRateHz) {
On 2015/09/22 12:10:10, magjed_webrtc wrote:
> ditto - Thread safety?

Done.

[henrika_webrtc, 2015-09-22 15:01:21]

Thanks magjed!

"One general comment is that it would be nice if we could send the parameters
defaultSampleRateHz, useWebRtcBasedAcousticEchoCanceler,
useWebRtcBasedAutomaticGainControl, and useWebRtcBasedNoiseSuppressor as
arguments to some constructor/init function instead of passing them via global
state, but I don't know this area of the code well enough to understand the pros
and cons."

Will keep that in mind and perhaps do that in a followup.

https://codereview.webrtc.org/1344563002/diff/160001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java
(right):

https://codereview.webrtc.org/1344563002/diff/160001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:66:
return WebRtcAudioUtils.runningOnJellyBeanOrHigher() &&
On 2015/09/22 14:19:04, magjed_webrtc wrote:
> && should be on the next line

Done.

https://codereview.webrtc.org/1344563002/diff/160001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:211:
// of the AEC effect is modified. Returns true if HW AEC is supported and
Discussed off-line. Keeping as is. False means "not supported, don't even try
setting the state".

[sophiechang-webrtc, 2015-09-22 20:15:33]

lgtm

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:43:
private static Boolean canUseAcousticEchoCanceler = null;
On 2015/09/22 11:57:04, henrika_webrtc wrote:
> I am not using them as tri-state here IMHO. The current design ensures that
> I don't do expensive work more than once even if the result is false.
> I would need an extra state otherwise to keep track of is the member is
defined
> or not.

That's basically what a tri-state is though... Will something that is actually
null be treated differently than false? (Probably more of a nit anyway)

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:343:
return type.equals(AudioEffect.EFFECT_TYPE_AEC)
On 2015/09/22 14:52:57, henrika_webrtc wrote:
> Not sure actually. Don't we need type.equals()?


actually to guard against NPEs, id do something like
return AudioEffect.EFFECT_TYPE_AEC.equals(type) ||
AudioEffect.EFFECT_TYPE_AGC.equals(type) ||
AudioEffect.EFFECT_TYPE_NS.equals(type);
since those AudioEffect(s) are guaranteed to be defined, while the UUID type is
not.

[henrika_webrtc, 2015-09-23 11:11:28]

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
File
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java
(right):

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:43:
private static Boolean canUseAcousticEchoCanceler = null;
Keeping as is then ;-)

https://codereview.webrtc.org/1344563002/diff/120001/webrtc/modules/audio_dev...
webrtc/modules/audio_device/android/java/src/org/webrtc/voiceengine/WebRtcAudioEffects.java:343:
return type.equals(AudioEffect.EFFECT_TYPE_AEC)
On 2015/09/22 20:15:32, sophiechang-webrtc wrote:
> On 2015/09/22 14:52:57, henrika_webrtc wrote:
> > Not sure actually. Don't we need type.equals()?
> 
> 
> actually to guard against NPEs, id do something like
> return AudioEffect.EFFECT_TYPE_AEC.equals(type) ||
> AudioEffect.EFFECT_TYPE_AGC.equals(type) ||
> AudioEffect.EFFECT_TYPE_NS.equals(type);
> since those AudioEffect(s) are guaranteed to be defined, while the UUID type
is
> not. 

Done.

[henrika_webrtc, 2015-09-23 11:37:46]

Adding pbos for talk/media/webrtc/

[henrika_webrtc, 2015-09-23 11:38:05]

henrika@webrtc.org changed reviewers:
+ pbos@webrtc.org

[pbos-webrtc, 2015-09-23 11:44:46]

lgtm for talk/

[henrika_webrtc, 2015-09-23 12:08:24]

TBR perkj for trivial change in talk/app/webrtc/test/fakeaudiocapturemodule.h

[henrika_webrtc, 2015-09-23 12:09:45]

Committed patchset #15 (id:280001) manually as
c14f5ff60fb0c42c97702de112a9e8f1eccba574 (presubmit successful).

[commit-bot: I haz the power, 2015-09-23 12:09:50]

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