Enable platform echo cancellation through the AudioRecord path.

Enable platform echo cancellation through the AudioRecord path.

Add a platform effects mask member to AudioParameters. This allows the
availability of platform effects (currently AEC) to be plumbed up to
MediaStreamDependencyFactory, where they can be reconciled with the
media constraints to determine if the effects should be enabled. When
this is the case, the constraints will be modified to disable the
corresponding software effect in PeerConnection.

The availability is controlled by a whitelist of device models in
AudioManagerAndroid, which for AEC, currently consists of Nexus 5 and
Nexus 7.

AudioManagerAndroid will use the AudioRecord path iff the platform
AEC is enabled.

TESTED=Using apprtc on a N5 and N7 (whitelisted):
- The AudioRecord input path is used.
- The platform AEC is enabled and the software AEC (in PeerConnection)
is disabled.
- Calls have good echo cancellation quality.

Using apprtc with ?audio=googEchoCancellation=false on a N5 and N7:
- The OpenSLES input path is used.
- Both the platform and software AEC are disabled.

Using apprtc on Nexus 4 (non-whitelisted):
- The OpenSLES input path is used.
- The platform AEC is disabled and the software AEC is enabled.

Using apprtc on Galaxy S2 (running ICS):
- The OpenSLES input path is used.

audio_android_unittest.cc passes on N5, N7 and Galaxy S2

TBR=jschuh

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240548

[ajm, 2013-12-06 04:29:22]

Ami: *.java changes
Tommi, Henrik A: everything

Shijing, Henrik E, Wei: added you to CC for awareness. Feel free to review if
you like, but not necessary.

I'm sure I'm missing owners for some files; I'll rope those in once we're agreed
on the approach.

I'll also modify audio_android_unittest.cc to exercise both the AudioRecord and
OpenSLES paths before landing.

[ajm, 2013-12-06 04:30:49]

https://codereview.chromium.org/99033003/diff/40001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/40001/media/audio/audio_paramet...
media/audio/audio_parameters.h:62: int frames_per_buffer, bool
use_platform_aec);
If we agree on this approach, I'll add use_platform_aec to the existing Reset().
I just didn't want to change the many call sites needlessly.

[henrika (OOO until Aug 14), 2013-12-06 06:56:54]

Looks like great stuff Andrew; but first: how does the EC performance differ for
the white listed devices with and without the hardware AEC? have you confirmed
that we really need it?

[ajm, 2013-12-06 06:59:09]

On 2013/12/06 06:56:54, henrika wrote:
> Looks like great stuff Andrew; but first: how does the EC performance differ
for
> the white listed devices with and without the hardware AEC? have you confirmed
> that we really need it?

Yes, it's considerably better than the current AECM solution in some quick
tests. I'll run some more tests tomorrow.

[henrika (OOO until Aug 14), 2013-12-06 07:02:26]

Can you quantify it? Clipping, double-talk etc. Better in all areas perhaps.
Sounds great in any case.

[ajm, 2013-12-06 07:15:23]

On 2013/12/06 07:02:26, henrika wrote:
> Can you quantify it? Clipping, double-talk etc. Better in all areas perhaps.
> Sounds great in any case.

Mainly that there aren't frequent horrible echo blips. Double-talk is somewhat
improved. I can make some recordings for comparison if you're interested.

[tommi (sloooow) - chröme, 2013-12-06 12:11:33]

Exciting stuff!

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:99: void
TransferConstraintToPlatformEffect(RTCMediaConstraints* constraints,
Please document all the arguments.  |platform_effect| in particular seems to be
an in+out argument which is non-obvious. In one case it's always overwritten (no
check for what value it was set to on input) and in the other case it's only
checked but not set...

Does |platform_effect| mean that the effect should be turned on or does it mean
that the effect is supported by the platform?

would it make sense to use two separate arguments?  Maybe the return value could
be used to indicate whether or not the effect should be turned on and the input
argument could indicate whether the platform actually supports it?
If you think that makes sense, I'd like to change the variable name to
platform_effect_supported or something like that since I'm having problems
parsing the code as is.

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:110: if
(*platform_effect) {
nit: you can save one scope:
} else if (*platform_effect) {

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:127:
MediaStreamDevice::AudioDeviceParameters* params) {
please document that both parameters are in+out and that the current value of
one will have an effect on the other.

Just from looking at the function, I would have assumed that I could use it like
this:

// Assume |constraints| is valid at this point.
// I want to use |constraints| to initialize my |params|.
// Since the function has the name "ToPlatformEffects"
// it's not obvious to me that constraints will be modified.

AudioDeviceParameters params;  // just use ctor initialized values for now.
TransferConstraintsToPlatformEffects(constraints, &params);

// BUG: If I understand correctly, what has happened here is
// that my |params| were unaffected by calling the Transfer function
// but my params actually may have caused |constraints| to be modified
// and that to me feels unexpected.

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:368: // the platform
AEC even if available.
This comment is good but I think it would be good to refactor the code so that
it will be more obvious what's going on.

https://codereview.chromium.org/99033003/diff/60001/media/audio/android/audio...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:196: if (params.use_platform_aec())
{
Just checking - is there no chance that we can have use_platform_aec as true
here when the platform doesn't support it?
If so, could we have a DCHECK to that effect?

https://codereview.chromium.org/99033003/diff/60001/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:200: } else {
nit: no need for 'else'

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
Can you add a TODO here to remove the other Reset() function and update all call
sites?

Can you also ping Dale (dalecurtis@) to have a look at this change?  Just in
case he has other ideas about where native audio effect flags should be.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:398:
boolean isWhitelisted = PLATFORM_AEC_MODEL_WHITELIST.contains(Build.MODEL);
nit: Since the list contains only two items, you probably don't need a HashSet
or static variable/array. You can also bail out sooner here if isAvailable()
returns false.
E.g.:

public static boolean isPlatformAECSupported() {
    if (Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN ||
        !AcousticEchoCanceler.isAvailable();) {
        // AcousticEchoCanceler was added in API level 16 (Jelly Bean).
        return false;
    }
    
    return Build.MODE.equals("Nexus 5") ||
           Build.MODE.equals("Nexus 7")
}

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioRecordInput.java
(right):

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:209:
boolean isEnabled = mAEC.getEnabled();
Can you add some comments for why we need to check if we succeeded in setting
the value?  I'm assuming that if we get here, we would have gotten an
AudioEffect.SUCCESS returned from setEnabled().

[henrika (OOO until Aug 14), 2013-12-06 12:52:21]

Cool, but is rather amazing how many files that must be affected for simply
turning one flag on or off.

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:368: // the platform
AEC even if available.
is it correct to assume that everything should work as before on all platforms
but on Android? What happens on all other platforms if we try to enable a
platform AEC?

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:117:
private static final Set<String> PLATFORM_AEC_MODEL_WHITELIST =
Guess you can follow this example instead of using HashSet. 
private static final String[] DEVICE_NAMES = new String[] {

or do as tommi suggests below...

But I like when you can find the list up here.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:391:
public static boolean isPlatformAECSupported() {
Would it make sense to name this API to something which makes it more clear that
we call AcousticEchoCanceler internally?

e.g. hasAcousticEchoCanceler()?

It is basically a thin wrapper on an existing Java API. We do so for e.g.
setSpeakerPhoneOn().

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioRecordInput.java
(right):

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:182: if
(AudioManagerAndroid.isPlatformAECSupported()) {
What happens if we create two streams and enable the AEC for one and disable it
for the other. Does that work? Session ID?

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:185:
Log.wtf(TAG, "AcousticEchoCanceler.create failed");
What does .wtf do?

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:188: try {
Am I the only one who feels that this try-catch style is difficult to read and
parse? Do we need it?

[ajm, 2013-12-06 19:06:43]

Responses to Java comments.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:391:
public static boolean isPlatformAECSupported() {
On 2013/12/06 12:52:21, henrika wrote:
> Would it make sense to name this API to something which makes it more clear
that
> we call AcousticEchoCanceler internally?
> 
> e.g. hasAcousticEchoCanceler()?
> 
> It is basically a thin wrapper on an existing Java API. We do so for e.g.
> setSpeakerPhoneOn().

Except that most (all?) devices will actually report isAvailable()==true, since
Android has a fallback SW AEC (webrtc's AECM of all things!). The more important
criterion here is the whitelist. I'm open to changing the function name
(suggestions?) but I don't think we should imply it's simply a wrapper over
isAvailable().

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:398:
boolean isWhitelisted = PLATFORM_AEC_MODEL_WHITELIST.contains(Build.MODEL);
On 2013/12/06 12:11:34, tommi wrote:
> nit: Since the list contains only two items, you probably don't need a HashSet
> or static variable/array. You can also bail out sooner here if isAvailable()
> returns false.
> E.g.:
> 
> public static boolean isPlatformAECSupported() {
>     if (Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN ||
>         !AcousticEchoCanceler.isAvailable();) {
>         // AcousticEchoCanceler was added in API level 16 (Jelly Bean).
>         return false;
>     }
>     
>     return Build.MODE.equals("Nexus 5") ||
>            Build.MODE.equals("Nexus 7")
> }

My initial implementation had all conditions in the return line. I only changed
it for logging purposes. I'm open to giving up the logging and returning to that
if you prefer.

Regarding the whitelist:
- I hope this list will be expanded in the future. For example, I think Nexus 4
can be added, but some more testing is needed.
- I like the clarity of using .contains() here. Is the concern that the HashSet
consumes unneeded memory?

I'm open to changing to your "A || B || ..." suggestion, just want to understand
the motivation.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioRecordInput.java
(right):

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:182: if
(AudioManagerAndroid.isPlatformAECSupported()) {
On 2013/12/06 12:52:21, henrika wrote:
> What happens if we create two streams and enable the AEC for one and disable
it
> for the other. Does that work? Session ID?

AFAIU it's only possible to open a single recording session on Android.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:185:
Log.wtf(TAG, "AcousticEchoCanceler.create failed");
On 2013/12/06 12:52:21, henrika wrote:
> What does .wtf do?

Intended for an error that should never happen (i.e. assert):
http://developer.android.com/reference/android/util/Log.html#wtf(java.lang.St...,
java.lang.String)

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:188: try {
On 2013/12/06 12:52:21, henrika wrote:
> Am I the only one who feels that this try-catch style is difficult to read and
> parse? Do we need it?

Yes, according to Ami, my friendly Java reviewer, we should be catching all
exceptions that calls can throw.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:209:
boolean isEnabled = mAEC.getEnabled();
On 2013/12/06 12:11:34, tommi wrote:
> Can you add some comments for why we need to check if we succeeded in setting
> the value?  I'm assuming that if we get here, we would have gotten an
> AudioEffect.SUCCESS returned from setEnabled().

Right, there's really no need for this. I was doing it for logging purposes, but
I can just as well log mUsePlatformAEC directly. Changed.

[Ami GONE FROM CHROMIUM, 2013-12-06 19:22:52]

Agree w/ other reviewer comments where not commented on them.
To avoid piling on will let tommi/henrika review quiesce before looking again
(please ping me when ready, or if you want me to pile on earlier ;)).

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/we...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.h:59: bool use_platform_aec);
I'm going to pretty sad if each platform effect means touching all the files
that this CL touches, and adding new bool use_platform_fooblart params each
time.  Time for a
struct PlatformEffects {
  bool echo_canceller;
};
and pass that around instead of the bool in all the new call-sites?

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
On 2013/12/06 12:11:34, tommi wrote:
> Can you add a TODO here to remove the other Reset() function and update all
call
> sites?

Better yet, can you delete the old form?

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:94: void set_use_platform_aec(bool use) {
Why let this be mutable instead of making it a ctor param?

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:391:
public static boolean isPlatformAECSupported() {
On 2013/12/06 12:52:21, henrika wrote:
> Would it make sense to name this API to something which makes it more clear
that
> we call AcousticEchoCanceler internally?
> 
> e.g. hasAcousticEchoCanceler()?
> 
> It is basically a thin wrapper on an existing Java API. We do so for e.g.
> setSpeakerPhoneOn().

hasAcousticEchoCanceler fails to connote that this method applies a whitelist. 
My bikeshed is painted shouldUseAcousticEchoCanceler.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:393: //
AcousticEchoCanceler was added in API level 16 (Jelly Bean).
Wouldn't this be accounted for by l.397?

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:400:
logd("Build.MODEL: " + Build.MODEL + " isWhitelisted: " + isWhitelisted);
I don't think these are worth spamming logcat for.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioRecordInput.java
(right):

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:182: if
(AudioManagerAndroid.isPlatformAECSupported()) {
This function was already consulted in the setting of the mUsePlatformAEC member
(from C++) so IMO this should be replaced with a test of that member.

That would also prevent a non-AEC-using stream from clobbering the AEC on an
already-opened stream, which seems better to me.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:185:
Log.wtf(TAG, "AcousticEchoCanceler.create failed");
On 2013/12/06 12:52:21, henrika wrote:
> What does .wtf do?

Crashes the process:
http://developer.android.com/reference/android/util/Log.html#wtf(java.lang.St...,
java.lang.String, java.lang.Throwable)

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:188: try {
On 2013/12/06 12:52:21, henrika wrote:
> Am I the only one who feels that this try-catch style is difficult to read and
> parse? 

No.  It seems crazy to me, too.

I suggest all the work happen in a single try/catch.
If you really meant wtf like you used it, then actually there's no point in the
catch at all - just let the JNI binding discover there's a pending exception and
CHECK-fail then.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:196: // Not
fatal.
lol that's not what the wtf() on the previous line implies!!

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:202: return
false;
unreachable

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:206: return
false;
unreachable

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:262:
mAEC.release();
mAEC refers to the mAudioRecord, so it should be released/null'd first, no?

(generally, open/close or start/stop pairs of functions should do things in the
opposite order from each other for this sort of reason)

[Ami GONE FROM CHROMIUM, 2013-12-06 19:24:47]

On Fri, Dec 6, 2013 at 11:06 AM, <ajm@chromium.org> wrote:

> https://codereview.chromium.org/99033003/diff/60001/media/
> base/android/java/src/org/chromium/media/AudioRecordInput.java#newcode188
> media/base/android/java/src/org/chromium/media/AudioRecordInput.java:188:
> try {
> On 2013/12/06 12:52:21, henrika wrote:
>
>> Am I the only one who feels that this try-catch style is difficult to
>>
> read and
>
>> parse? Do we need it?
>>
>
> Yes, according to Ami, my friendly Java reviewer, we should be catching
> all exceptions that calls can throw.


FTR, my comment was in the context of "if you don't catch thrown
exceptions, then the auto-generated JNI bindings machinery will CHECK-fail
when it notices a pending exception upon returning to C++".  If all you do
in your catch block is Log.wtf() then there's no need for the catch.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ajm, 2013-12-06 19:31:23]

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/we...
File content/renderer/media/webrtc_audio_capturer.h (right):

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/we...
content/renderer/media/webrtc_audio_capturer.h:59: bool use_platform_aec);
On 2013/12/06 19:22:53, Ami Fischman wrote:
> I'm going to pretty sad if each platform effect means touching all the files
> that this CL touches, and adding new bool use_platform_fooblart params each
> time.  Time for a
> struct PlatformEffects {
>   bool echo_canceller;
> };
> and pass that around instead of the bool in all the new call-sites?

Agreed, I was thinking the same thing after reading Tommi's comments.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioRecordInput.java
(right):

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:196: // Not
fatal.
On 2013/12/06 19:22:53, Ami Fischman wrote:
> lol that's not what the wtf() on the previous line implies!!

Yeah noticed that myself :(

Changed to Log.w in the next patchset.

[ajm, 2013-12-06 19:32:02]

On 2013/12/06 19:24:47, Ami Fischman wrote:
> On Fri, Dec 6, 2013 at 11:06 AM, <mailto:ajm@chromium.org> wrote:
> 
> > https://codereview.chromium.org/99033003/diff/60001/media/
> > base/android/java/src/org/chromium/media/AudioRecordInput.java#newcode188
> > media/base/android/java/src/org/chromium/media/AudioRecordInput.java:188:
> > try {
> > On 2013/12/06 12:52:21, henrika wrote:
> >
> >> Am I the only one who feels that this try-catch style is difficult to
> >>
> > read and
> >
> >> parse? Do we need it?
> >>
> >
> > Yes, according to Ami, my friendly Java reviewer, we should be catching
> > all exceptions that calls can throw.
> 
> 
> FTR, my comment was in the context of "if you don't catch thrown
> exceptions, then the auto-generated JNI bindings machinery will CHECK-fail
> when it notices a pending exception upon returning to C++".  If all you do
> in your catch block is Log.wtf() then there's no need for the catch.

Got it. I'll see what I can clean up.

[henrika (OOO until Aug 14), 2013-12-06 22:51:57]

I can't wait to try it out. Great description, clean code and nice structure.
The rest is just details.

LGTM.

https://codereview.chromium.org/99033003/diff/80001/media/audio/android/audio...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/99033003/diff/80001/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:120:
base::android::AttachCurrentThread()));
Any chance you would consider following the example of an old programmer and use
caching here: JNIEnv* env = AttachCurrentThread();

https://codereview.chromium.org/99033003/diff/80001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/99033003/diff/80001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:117:
private static final Set<String> PLATFORM_AEC_MODEL_WHITELIST =
Just FYI, I was once told that every byte counts, especially on mobile
platforms, and we all know that HashSets is a rather new invention which we can
live without. Your choice.

[ajm, 2013-12-10 06:37:15]

Dale, could you have a look at the new PlatformEffects member in
AudioParameters? Tommi thought we should check with you, in case you had other
ideas about where this should go.

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/me...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:99: void
TransferConstraintToPlatformEffect(RTCMediaConstraints* constraints,
On 2013/12/06 12:11:34, tommi wrote:
> Please document all the arguments.  |platform_effect| in particular seems to
be
> an in+out argument which is non-obvious. In one case it's always overwritten
(no
> check for what value it was set to on input) and in the other case it's only
> checked but not set...
> 
> Does |platform_effect| mean that the effect should be turned on or does it
mean
> that the effect is supported by the platform?
> 
> would it make sense to use two separate arguments?  Maybe the return value
could
> be used to indicate whether or not the effect should be turned on and the
input
> argument could indicate whether the platform actually supports it?
> If you think that makes sense, I'd like to change the variable name to
> platform_effect_supported or something like that since I'm having problems
> parsing the code as is.

I tried returning the output effect and using just an input parameter, but it
served to complicate the one call site (since I would set the return to the same
variable as I used for input anyway).

I left them both as input/output parameters, but hopefully with the rename and
new comments, it's clear what the intent is. LMK.

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:110: if
(*platform_effect) {
On 2013/12/06 12:11:34, tommi wrote:
> nit: you can save one scope:
> } else if (*platform_effect) {

Done.

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:127:
MediaStreamDevice::AudioDeviceParameters* params) {
On 2013/12/06 12:11:34, tommi wrote:
> please document that both parameters are in+out and that the current value of
> one will have an effect on the other.
> 
> Just from looking at the function, I would have assumed that I could use it
like
> this:
> 
> // Assume |constraints| is valid at this point.
> // I want to use |constraints| to initialize my |params|.
> // Since the function has the name "ToPlatformEffects"
> // it's not obvious to me that constraints will be modified.
> 
> AudioDeviceParameters params;  // just use ctor initialized values for now.
> TransferConstraintsToPlatformEffects(constraints, &params);
> 
> // BUG: If I understand correctly, what has happened here is
> // that my |params| were unaffected by calling the Transfer function
> // but my params actually may have caused |constraints| to be modified
> // and that to me feels unexpected.

Done. Hopefully you think it's sufficiently clear now ;)

https://codereview.chromium.org/99033003/diff/60001/content/renderer/media/me...
content/renderer/media/media_stream_dependency_factory.cc:368: // the platform
AEC even if available.
On 2013/12/06 12:52:21, henrika wrote:
> is it correct to assume that everything should work as before on all platforms
> but on Android? What happens on all other platforms if we try to enable a
> platform AEC?

If an AudioManager claims that a platform AEC exists where it doesn't then we'll
have neither platform (HW) nor SW AEC enabled.

But, yes, it's correct to assume that everything should work as before, since no
other platform is claiming existence of effects.

https://codereview.chromium.org/99033003/diff/60001/media/audio/android/audio...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:196: if (params.use_platform_aec())
{
On 2013/12/06 12:11:34, tommi wrote:
> Just checking - is there no chance that we can have use_platform_aec as true
> here when the platform doesn't support it?
> If so, could we have a DCHECK to that effect?

Right, that would be a programming error. For testing purposes, however, we want
to be able to set this when shouldUse<Effect> returned false, so as explained we
only DCHECK against the SDK version.

https://codereview.chromium.org/99033003/diff/60001/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:200: } else {
On 2013/12/06 12:11:34, tommi wrote:
> nit: no need for 'else'

Done.

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
On 2013/12/06 19:22:53, Ami Fischman wrote:
> On 2013/12/06 12:11:34, tommi wrote:
> > Can you add a TODO here to remove the other Reset() function and update all
> call
> > sites?
> 
> Better yet, can you delete the old form?

I don't actually need to use Reset(), as all of the 16 calls to it but one don't
care about the effects.

Do you think that's dangerous? Someone adding a call to Reset() in the path that
does care about effects is more likely to miss that they need to set it
properly.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:117:
private static final Set<String> PLATFORM_AEC_MODEL_WHITELIST =
On 2013/12/06 12:52:21, henrika wrote:
> Guess you can follow this example instead of using HashSet. 
> private static final String[] DEVICE_NAMES = new String[] {
> 
> or do as tommi suggests below...
> 
> But I like when you can find the list up here.

Done.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:391:
public static boolean isPlatformAECSupported() {
On 2013/12/06 19:22:53, Ami Fischman wrote:
> On 2013/12/06 12:52:21, henrika wrote:
> > Would it make sense to name this API to something which makes it more clear
> that
> > we call AcousticEchoCanceler internally?
> > 
> > e.g. hasAcousticEchoCanceler()?
> > 
> > It is basically a thin wrapper on an existing Java API. We do so for e.g.
> > setSpeakerPhoneOn().
> 
> hasAcousticEchoCanceler fails to connote that this method applies a whitelist.

> My bikeshed is painted shouldUseAcousticEchoCanceler.

Sounds good.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:393: //
AcousticEchoCanceler was added in API level 16 (Jelly Bean).
On 2013/12/06 19:22:53, Ami Fischman wrote:
> Wouldn't this be accounted for by l.397?

As I understand it, we'll get a crash if we try to execute anything unavailable
in the runtime SDK. Since Chrome's minSdkVersion is set to 14 (ICS), we need to
guard the new functionality.

I've added comments elsewhere to explain that this class should only be used on
Jelly Bean and up.

None of the devices I have ATM can be flashed to ICS. I'll see if I can pick up
an older device tomorrow to ensure this works fine.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:398:
boolean isWhitelisted = PLATFORM_AEC_MODEL_WHITELIST.contains(Build.MODEL);
Changed as recommended now.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:400:
logd("Build.MODEL: " + Build.MODEL + " isWhitelisted: " + isWhitelisted);
On 2013/12/06 19:22:53, Ami Fischman wrote:
> I don't think these are worth spamming logcat for.

Done.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioRecordInput.java
(right):

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:182: if
(AudioManagerAndroid.isPlatformAECSupported()) {
On 2013/12/06 19:22:53, Ami Fischman wrote:
> This function was already consulted in the setting of the mUsePlatformAEC
member
> (from C++) so IMO this should be replaced with a test of that member.
> 
> That would also prevent a non-AEC-using stream from clobbering the AEC on an
> already-opened stream, which seems better to me.

When we add other effects it will be possible to disable an effect (now, this
class is only created when mUsePlatformAEC is true).

What I really want to do here is check isAvailable(), not consult the whitelist,
to ensure the following calls should succeed.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:188: try {
On 2013/12/06 19:22:53, Ami Fischman wrote:
> On 2013/12/06 12:52:21, henrika wrote:
> > Am I the only one who feels that this try-catch style is difficult to read
and
> > parse? 
> 
> No.  It seems crazy to me, too.
> 
> I suggest all the work happen in a single try/catch.
> If you really meant wtf like you used it, then actually there's no point in
the
> catch at all - just let the JNI binding discover there's a pending exception
and
> CHECK-fail then.

I've attempted to clean this up appropriately throughout the class. Let me know
what you think.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:202: return
false;
On 2013/12/06 19:22:53, Ami Fischman wrote:
> unreachable

Done.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:206: return
false;
On 2013/12/06 19:22:53, Ami Fischman wrote:
> unreachable

Done.

https://codereview.chromium.org/99033003/diff/60001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:262:
mAEC.release();
On 2013/12/06 19:22:53, Ami Fischman wrote:
> mAEC refers to the mAudioRecord, so it should be released/null'd first, no?
> 
> (generally, open/close or start/stop pairs of functions should do things in
the
> opposite order from each other for this sort of reason)

Good point, done.

https://codereview.chromium.org/99033003/diff/80001/media/audio/android/audio...
File media/audio/android/audio_manager_android.cc (right):

https://codereview.chromium.org/99033003/diff/80001/media/audio/android/audio...
media/audio/android/audio_manager_android.cc:120:
base::android::AttachCurrentThread()));
On 2013/12/06 22:51:57, henrika wrote:
> Any chance you would consider following the example of an old programmer and
use
> caching here: JNIEnv* env = AttachCurrentThread();

Absolutely :)

https://codereview.chromium.org/99033003/diff/80001/media/base/android/java/s...
File media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java
(right):

https://codereview.chromium.org/99033003/diff/80001/media/base/android/java/s...
media/base/android/java/src/org/chromium/media/AudioManagerAndroid.java:117:
private static final Set<String> PLATFORM_AEC_MODEL_WHITELIST =
On 2013/12/06 22:51:57, henrika wrote:
> Just FYI, I was once told that every byte counts, especially on mobile
> platforms, and we all know that HashSets is a rather new invention which we
can
> live without. Your choice.

Thanks, removed it.

https://codereview.chromium.org/99033003/diff/120001/content/public/common/me...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/99033003/diff/120001/content/public/common/me...
content/public/common/media_stream_request.h:16: #include
"media/audio/audio_parameters.h"
This trips a presubmit check:
"You added one or more #includes that violate checkdeps rules."

I suppose I'll have to copy over the definition of PlatformEffects? I'll do that
tomorrow.

https://codereview.chromium.org/99033003/diff/120001/media/audio/android/audi...
File media/audio/android/audio_android_unittest.cc (right):

https://codereview.chromium.org/99033003/diff/120001/media/audio/android/audi...
media/audio/android/audio_android_unittest.cc:467: if
(base::android::BuildInfo::GetInstance()->sdk_int() >= 16)
Although this works in my testing, I suspect it might trigger the static
initialization order fiasco. Anyone sure if it's safe?

If not, I'll probably have to always run both sets of tests, and just disable
the AudioRecord path as the test fires.

[Ami GONE FROM CHROMIUM, 2013-12-10 19:31:42]

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
On 2013/12/10 06:37:16, ajm wrote:
> On 2013/12/06 19:22:53, Ami Fischman wrote:
> > On 2013/12/06 12:11:34, tommi wrote:
> > > Can you add a TODO here to remove the other Reset() function and update
all
> > call
> > > sites?
> > 
> > Better yet, can you delete the old form?
> 
> I don't actually need to use Reset(), as all of the 16 calls to it but one
don't
> care about the effects.
> 
> Do you think that's dangerous? Someone adding a call to Reset() in the path
that
> does care about effects is more likely to miss that they need to set it
> properly. 

I think it would be better to make use_platform_aec a ctor arg and a const
member and drop set_use_platform_aec, which would allow you to avoid this
question.  (originally commented to this effect in a comment you didn't respond
to at l.94 below).

[ajm, 2013-12-10 20:13:04]

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
On 2013/12/10 19:31:43, Ami Fischman wrote:
> I think it would be better to make use_platform_aec a ctor arg and a const
> member and drop set_use_platform_aec, which would allow you to avoid this
> question.  (originally commented to this effect in a comment you didn't
respond
> to at l.94 below).

Sorry, missed that comment.

I like your suggestion; the only potential wrinkle I can see is that here:
https://code.google.com/p/chromium/codesearch#chromium/src/content/common/med...

we'll have to construct a new AudioParameters and copy it to the input
parameter, e.g.

AudioParameters params(...)
*r = params;

Do you think that's fine?

[Ami GONE FROM CHROMIUM, 2013-12-10 20:19:43]

Yes.


On Tue, Dec 10, 2013 at 12:13 PM, <ajm@chromium.org> wrote:

>
> https://codereview.chromium.org/99033003/diff/60001/media/
> audio/audio_parameters.h
> File media/audio/audio_parameters.h (right):
>
> https://codereview.chromium.org/99033003/diff/60001/media/
> audio/audio_parameters.h#newcode59
> media/audio/audio_parameters.h:59: void Reset(Format format,
> ChannelLayout channel_layout,
> On 2013/12/10 19:31:43, Ami Fischman wrote:
>
>> I think it would be better to make use_platform_aec a ctor arg and a
>>
> const
>
>> member and drop set_use_platform_aec, which would allow you to avoid
>>
> this
>
>> question.  (originally commented to this effect in a comment you
>>
> didn't respond
>
>> to at l.94 below).
>>
>
> Sorry, missed that comment.
>
> I like your suggestion; the only potential wrinkle I can see is that
> here:
> https://code.google.com/p/chromium/codesearch#chromium/
> src/content/common/media/media_param_traits.cc&sq=
> package:chromium&type=cs&l=44
>
> we'll have to construct a new AudioParameters and copy it to the input
> parameter, e.g.
>
> AudioParameters params(...)
> *r = params;
>
> Do you think that's fine?
>
> https://codereview.chromium.org/99033003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[tommi (sloooow) - chröme, 2013-12-10 20:24:10]

OK with me too.  I've got some minor comments on the way but won't be able
to finish the review until tomorrow.
If other reviewers l.gtm the change today, feel free to land and
address/ignore my comments in a later cl.


On Tue, Dec 10, 2013 at 9:19 PM, Ami Fischman <fischman@chromium.org> wrote:

> Yes.
>
>
> On Tue, Dec 10, 2013 at 12:13 PM, <ajm@chromium.org> wrote:
>
>>
>> https://codereview.chromium.org/99033003/diff/60001/media/
>> audio/audio_parameters.h
>> File media/audio/audio_parameters.h (right):
>>
>> https://codereview.chromium.org/99033003/diff/60001/media/
>> audio/audio_parameters.h#newcode59
>> media/audio/audio_parameters.h:59: void Reset(Format format,
>> ChannelLayout channel_layout,
>> On 2013/12/10 19:31:43, Ami Fischman wrote:
>>
>>> I think it would be better to make use_platform_aec a ctor arg and a
>>>
>> const
>>
>>> member and drop set_use_platform_aec, which would allow you to avoid
>>>
>> this
>>
>>> question.  (originally commented to this effect in a comment you
>>>
>> didn't respond
>>
>>> to at l.94 below).
>>>
>>
>> Sorry, missed that comment.
>>
>> I like your suggestion; the only potential wrinkle I can see is that
>> here:
>> https://code.google.com/p/chromium/codesearch#chromium/
>> src/content/common/media/media_param_traits.cc&sq=
>> package:chromium&type=cs&l=44
>>
>> we'll have to construct a new AudioParameters and copy it to the input
>> parameter, e.g.
>>
>> AudioParameters params(...)
>> *r = params;
>>
>> Do you think that's fine?
>>
>> https://codereview.chromium.org/99033003/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2013-12-11 00:08:47]

Ah, AudioParameters is well on its way to becoming a generic data transport :-/ 
At least I think instead of a structure it might be simpler to just have a
bitmask of effects.

[ajm, 2013-12-11 01:15:16]

On 2013/12/11 00:08:47, DaleCurtis wrote:
> Ah, AudioParameters is well on its way to becoming a generic data transport
:-/ 
> At least I think instead of a structure it might be simpler to just have a
> bitmask of effects.

That would at least solve the checkdeps problem I'm hitting using the struct.
audio_parameters.h and media_stream_request.h are not allowed to include the
other.

I'll start switching to a bitmask. (Other reviewers, pipe up if you disagree :)

[Ami GONE FROM CHROMIUM, 2013-12-11 01:21:15]

Are you really that sure that you won't want effects that take parameters?


On Tue, Dec 10, 2013 at 5:15 PM, <ajm@chromium.org> wrote:

> On 2013/12/11 00:08:47, DaleCurtis wrote:
>
>> Ah, AudioParameters is well on its way to becoming a generic data
>> transport
>>
> :-/
>
>> At least I think instead of a structure it might be simpler to just have a
>> bitmask of effects.
>>
>
> That would at least solve the checkdeps problem I'm hitting using the
> struct.
> audio_parameters.h and media_stream_request.h are not allowed to include
> the
> other.
>
> I'll start switching to a bitmask. (Other reviewers, pipe up if you
> disagree :)
>
> https://codereview.chromium.org/99033003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ajm, 2013-12-11 01:39:54]

On 2013/12/11 01:21:15, Ami Fischman wrote:
> Are you really that sure that you won't want effects that take parameters?

Good point, no I'm not certain.

At least all of the Android input effects that are motivating this work don't
take any parameters. But if we wanted to expand it to output effects, say:
http://developer.android.com/reference/android/media/audiofx/Equalizer.html

there are many available parameters. And it might be that input effects on other
platforms take parameters.

Dale, what do you think? I have no current plans to add output effects or
additional platforms, but it could certainly happen in the future.

[DaleCurtis, 2013-12-11 01:49:08]

I think it's already kind of iffy to add effects to AudioParameters, if you're
going to have parameterized effects, I'd recommend considering a new means of
passing them outside of AudioParameters.

[ajm, 2013-12-11 02:10:52]

On 2013/12/11 01:49:08, DaleCurtis wrote:
> I think it's already kind of iffy to add effects to AudioParameters, if you're
> going to have parameterized effects, I'd recommend considering a new means of
> passing them outside of AudioParameters.

Understood. My preference then is to keep it in AudioParameters now as a mask,
and if/when we start using parameterized effects, refactoring to plumb through a
new type.

Other reviewers: what do you think?

[Ami GONE FROM CHROMIUM, 2013-12-11 02:20:08]

On Tue, Dec 10, 2013 at 6:10 PM, <ajm@chromium.org> wrote:

> Other reviewers: what do you think?
>

Defer to Dale.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ajm, 2013-12-11 02:48:03]

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
Moved to setting through constructors in patch set 5. However, I couldn't make
effects_ a const member, as assignment is used for AudioParameters, including
now in my change to media_param_traits.cc. Do you still prefer the constructor
approach?

[Ami GONE FROM CHROMIUM, 2013-12-11 05:25:07]

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
On 2013/12/11 02:48:04, ajm wrote:
> Moved to setting through constructors in patch set 5. However, I couldn't make
> effects_ a const member, as assignment is used for AudioParameters, including
> now in my change to media_param_traits.cc. Do you still prefer the constructor
> approach? 

What prevents you from reading into a temporary in traits.cc and using an
explicit constructor for PlatformEffect to make its effects_ member a const?

[henrika (OOO until Aug 14), 2013-12-11 08:24:22]

> Other reviewers: what do you think?
>

Defer to Dale.

Andrew, forgot to ask: regarding the white list, where are we on e.g. Motorola
devices?

[tommi (sloooow) - chröme, 2013-12-11 12:14:36]

lgtm. sorry for the delay yesterday.  I've got one request for the "reconcile"
methods since they exceed my mental capacity! :)

https://codereview.chromium.org/99033003/diff/120001/content/public/common/me...
File content/public/common/media_stream_request.h (right):

https://codereview.chromium.org/99033003/diff/120001/content/public/common/me...
content/public/common/media_stream_request.h:16: #include
"media/audio/audio_parameters.h"
On 2013/12/10 06:37:16, ajm wrote:
> This trips a presubmit check:
> "You added one or more #includes that violate checkdeps rules."
> 
> I suppose I'll have to copy over the definition of PlatformEffects? I'll do
that
> tomorrow.

Yes unfortunately.  That's why we're not using AudioParameters for sample rate,
channel layout etc here.  It would be good if we didn't have to have this here
though since I'm not convinced these details need to be a part of the public
content interface... hmm... no action on that in this cl though.

https://codereview.chromium.org/99033003/diff/120001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/99033003/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:99: typedef
media::AudioParameters::PlatformEffects PlatformEffects;
Is it a good idea to create a new type for this?  I'm guessing you're only
interested in saving on keystrokes.  Is |using media::AudioParameters| at the
top of the file good enough?

https://codereview.chromium.org/99033003/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:102: // |effect| is
the state of the corresponding platform effect.
This function is still confusing to me since both |key| and |constraints| will
change the state of the other and it's not clear which one wins.

What about removing the ReconcileConstraintAndPlatformEffect +
ReconcileConstraintsAndPlatformEffects, and instead do:

if (device_info.device.input.effects.echo_canceller) {
  // Try to set the matching mandatory constraint.
  // If the feature has been disabled via constraints,
  // echo_canceller will be set to false.
  device_info.device.input.effects.echo_canceller =
      platform_constraints.AddMandatory(
          webrtc::MediaConstraintsInterface::kEchoCancellation,
          webrtc::MediaConstraintsInterface::kValueTrue, false);
}


AddMandatory() returns true when it sets the constraint or when the constraint
is already set to the same value.  It return false (since I pass false for
|override_if_exists|) if the constraint has been set already but to a different
value.

This is much less code + comments and I think it accomplishes what you want
(unless of course Iḿ completely missing the point which is always possible).

https://codereview.chromium.org/99033003/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:110: if
(!webrtc::FindConstraint(constraints, key, &value, NULL) ||
nit: this fits on one line:
  if (!webrtc::FindConstraint(constraints, key, &value, NULL) || !value) {

https://codereview.chromium.org/99033003/diff/120001/content/renderer/media/w...
File content/renderer/media/webrtc_audio_capturer.cc (right):

https://codereview.chromium.org/99033003/diff/120001/content/renderer/media/w...
content/renderer/media/webrtc_audio_capturer.cc:131: bool
WebRtcAudioCapturer::Initialize(int render_view_id,
nit: move render_view_id on a new line as well

https://codereview.chromium.org/99033003/diff/120001/media/audio/audio_parame...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/120001/media/audio/audio_parame...
media/audio/audio_parameters.h:51: bool operator==(const PlatformEffects& other)
const {
document why you need the comparison operator. Usually we don't override
operators unless there's a specific need to (it goes against the style guide).
If you don't need it, you can still have an IsEqual method.

[ajm, 2013-12-11 17:00:40]

On 2013/12/11 08:24:22, henrika wrote:
> > Other reviewers: what do you think?
> >
> 
> Defer to Dale.
> 
> Andrew, forgot to ask: regarding the white list, where are we on e.g. Motorola
> devices?

We have a skylab now in MTV that has a good selection of devices. I'll be
testing the Samsung Galaxy S3/S4 first as they appear to be among the most
popular devices. The skylab doesn't have a Moto X yet; I'll see if they can add
one.

[ajm, 2013-12-11 17:46:48]

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
On 2013/12/11 05:25:08, Ami Fischman wrote:
> On 2013/12/11 02:48:04, ajm wrote:
> > Moved to setting through constructors in patch set 5. However, I couldn't
make
> > effects_ a const member, as assignment is used for AudioParameters,
including
> > now in my change to media_param_traits.cc. Do you still prefer the
constructor
> > approach? 
> 
> What prevents you from reading into a temporary in traits.cc and using an
> explicit constructor for PlatformEffect to make its effects_ member a const?

I assume you mean "explicit constructor for AudioParameters". That's what I'm
doing now, right? The problem comes when I try to assign the explicitly
constructed |params| to |*r|. The latter can't be assigned to because effects_
is a const member.

I may be totally missing something here.

https://codereview.chromium.org/99033003/diff/120001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/99033003/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:102: // |effect| is
the state of the corresponding platform effect.
On 2013/12/11 12:14:36, tommi wrote:
> This function is still confusing to me since both |key| and |constraints| will
> change the state of the other and it's not clear which one wins.
> 
> What about removing the ReconcileConstraintAndPlatformEffect +
> ReconcileConstraintsAndPlatformEffects, and instead do:
> 
> if (device_info.device.input.effects.echo_canceller) {
>   // Try to set the matching mandatory constraint.
>   // If the feature has been disabled via constraints,
>   // echo_canceller will be set to false.
>   device_info.device.input.effects.echo_canceller =
>       platform_constraints.AddMandatory(
>           webrtc::MediaConstraintsInterface::kEchoCancellation,
>           webrtc::MediaConstraintsInterface::kValueTrue, false);
> }
> 
> 
> AddMandatory() returns true when it sets the constraint or when the constraint
> is already set to the same value.  It return false (since I pass false for
> |override_if_exists|) if the constraint has been set already but to a
different
> value.
> 
> This is much less code + comments and I think it accomplishes what you want
> (unless of course Iḿ completely missing the point which is always possible).

That works, and I agree is much cleaner.

https://codereview.chromium.org/99033003/diff/120001/media/audio/audio_parame...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/120001/media/audio/audio_parame...
media/audio/audio_parameters.h:51: bool operator==(const PlatformEffects& other)
const {
On 2013/12/11 12:14:36, tommi wrote:
> document why you need the comparison operator. Usually we don't override
> operators unless there's a specific need to (it goes against the style guide).
> If you don't need it, you can still have an IsEqual method.

It was only to support the AudioParameters comparison operator, but I could just
as well call an IsEqual there.

[Ami GONE FROM CHROMIUM, 2013-12-11 22:11:37]

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
On 2013/12/11 17:46:49, ajm wrote:
> On 2013/12/11 05:25:08, Ami Fischman wrote:
> > On 2013/12/11 02:48:04, ajm wrote:
> > > Moved to setting through constructors in patch set 5. However, I couldn't
> make
> > > effects_ a const member, as assignment is used for AudioParameters,
> including
> > > now in my change to media_param_traits.cc. Do you still prefer the
> constructor
> > > approach? 
> > 
> > What prevents you from reading into a temporary in traits.cc and using an
> > explicit constructor for PlatformEffect to make its effects_ member a const?
> 
> I assume you mean "explicit constructor for AudioParameters". That's what I'm
> doing now, right? The problem comes when I try to assign the explicitly
> constructed |params| to |*r|. The latter can't be assigned to because effects_
> is a const member.
> 
> I may be totally missing something here.

Perhaps that something is a copy ctor for PlatformEffects ?

[Ami GONE FROM CHROMIUM, 2013-12-11 22:20:33]

LGTM % nits 
(only did a pass on .java files since I realized that's what you asked for
originally, and others have covered the C++ side)

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
File media/base/android/java/src/org/chromium/media/AudioRecordInput.java
(right):

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:177: if
(AcousticEchoCanceler.isAvailable()) {
I thought you were worried about calling this b/c it would crash on old
devices/SDKs?

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:180:
Log.wtf(TAG, "AcousticEchoCanceler.create failed");
Can this trigger e.g. if there are multiple AECs concurrently operating?  
Should that crash the tab or just turn AEC off?

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:184:
Log.d(TAG, "AcousticEchoCanceler " +
Why should this be emitted to logcat on release builds?

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:236:
mAEC.release();
mAEC refers to mAudioRecord so it should be released _first_.
(in general release resources in reverse order of their acquisition)

[ajm, 2013-12-12 01:51:26]

Please take a look at...

James: OWNER approval for minor change to renderer_webkitplatformsupport_impl.cc
Jói: OWNER approval for media_stream_request.h

Ami: The Java changes and copy/assignment question.
Dale: The bitmask change in audio_parameters.h.
Tommi: The updated "reconcile" code.

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
On 2013/12/11 22:11:38, Ami Fischman wrote:
> Perhaps that something is a copy ctor for PlatformEffects ?

This would require an assignment operator, right? And we still can't assign to
the const member.

It's changed to a bitmask now, so things are a bit different, but please let me
know if I'm misinterpreting, and there's some way to use assignment when the
type has a const member.

https://codereview.chromium.org/99033003/diff/120001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/99033003/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:99: typedef
media::AudioParameters::PlatformEffects PlatformEffects;
On 2013/12/11 12:14:36, tommi wrote:
> Is it a good idea to create a new type for this?  I'm guessing you're only
> interested in saving on keystrokes.  Is |using media::AudioParameters| at the
> top of the file good enough?

Removed.

https://codereview.chromium.org/99033003/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:102: // |effect| is
the state of the corresponding platform effect.
On 2013/12/11 17:46:49, ajm wrote:
> That works, and I agree is much cleaner.

Ugh, on further reflection, it doesn't work. First that's not how AddMandatory()
works: when |override_if_exists| is false, it returns false if the key is found,
regardless of the value. It only returns true if the key wasn't found.

Even under the previous assumption this doesn't quite do what we want. If both
the effect is available and the constraint is enabled, we want to disable the
constraint in order to disable the SW effect.

I've removed these functions and tried to make the implementation as clear as
possible inline.

https://codereview.chromium.org/99033003/diff/120001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:110: if
(!webrtc::FindConstraint(constraints, key, &value, NULL) ||
On 2013/12/11 12:14:36, tommi wrote:
> nit: this fits on one line:
>   if (!webrtc::FindConstraint(constraints, key, &value, NULL) || !value) {

Removed.

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
File media/base/android/java/src/org/chromium/media/AudioRecordInput.java
(right):

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:177: if
(AcousticEchoCanceler.isAvailable()) {
On 2013/12/11 22:20:34, Ami Fischman wrote:
> I thought you were worried about calling this b/c it would crash on old
> devices/SDKs?

Right, I've documented in the native class that AudioRecordInputStream should
only be created on Jelly Bean or higher. I didn't want to duplicate the checks,
but do you think it's better to be defensive here and check before calling this?

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:180:
Log.wtf(TAG, "AcousticEchoCanceler.create failed");
On 2013/12/11 22:20:34, Ami Fischman wrote:
> Can this trigger e.g. if there are multiple AECs concurrently operating?  
> Should that crash the tab or just turn AEC off?

Good question; I tested and neither the OpenSLES or AudioRecord paths will
support multiple recording sessions (that is, you don't get audio). It was a
good test, as it motivated bailing out of run() early above on the error
received in this situation.

In any case, this doesn't trigger, but even if it did in an as-yet-unknown
situation, I'm now leaning towards being in a disabled AEC state rather than
crashing. Changed to Log.e() and below.

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:184:
Log.d(TAG, "AcousticEchoCanceler " +
On 2013/12/11 22:20:34, Ami Fischman wrote:
> Why should this be emitted to logcat on release builds?

Agree it shouldn't. Adopted a pattern from AudioManagerAndroid.java to not log
it by default. (And removed the catch, because I don't care about it crashing
when I've set DEBUG=true).

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:236:
mAEC.release();
On 2013/12/11 22:20:34, Ami Fischman wrote:
> mAEC refers to mAudioRecord so it should be released _first_.
> (in general release resources in reverse order of their acquisition)

That's what's happening here, right?

https://codereview.chromium.org/99033003/diff/180001/media/audio/audio_parame...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/180001/media/audio/audio_parame...
media/audio/audio_parameters.h:58: AudioParameters(Format format, ChannelLayout
channel_layout,
This existing constructor didn't have many call sites, so I opted to add a
parameter to it rather than creating a new one.

(Also, with the addition of an int to the new constructor, the prototypes were
ambiguous)

[Ami GONE FROM CHROMIUM, 2013-12-12 02:01:19]

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
On 2013/12/12 01:51:27, ajm wrote:
> On 2013/12/11 22:11:38, Ami Fischman wrote:
> > Perhaps that something is a copy ctor for PlatformEffects ?
> 
> This would require an assignment operator, right? And we still can't assign to
> the const member.
> 
> It's changed to a bitmask now, so things are a bit different, but please let
me
> know if I'm misinterpreting, and there's some way to use assignment when the
> type has a const member.

You're misinterpreting.
"Copy constructor" is a term of art in C++ and is different to assignment
operator.  Because the copy ctor is a ctor it gets to assign to const members.
http://en.wikipedia.org/wiki/Copy_constructor

(I don't actually believe you don't know this; I'm just being explicit to make
sure we're communicating :))

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
File media/base/android/java/src/org/chromium/media/AudioRecordInput.java
(right):

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:177: if
(AcousticEchoCanceler.isAvailable()) {
On 2013/12/12 01:51:27, ajm wrote:
> On 2013/12/11 22:20:34, Ami Fischman wrote:
> > I thought you were worried about calling this b/c it would crash on old
> > devices/SDKs?
> 
> Right, I've documented in the native class that AudioRecordInputStream should
> only be created on Jelly Bean or higher. I didn't want to duplicate the
checks,
> but do you think it's better to be defensive here and check before calling
this?

No, fine to prevent at the outermost layer of the onion.

https://codereview.chromium.org/99033003/diff/140001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:236:
mAEC.release();
On 2013/12/12 01:51:27, ajm wrote:
> On 2013/12/11 22:20:34, Ami Fischman wrote:
> > mAEC refers to mAudioRecord so it should be released _first_.
> > (in general release resources in reverse order of their acquisition)
> 
> That's what's happening here, right?

lol, yes.  Sorry.  Misread the null-check code above.

https://codereview.chromium.org/99033003/diff/200001/media/base/android/java/...
File media/base/android/java/src/org/chromium/media/AudioRecordInput.java
(right):

https://codereview.chromium.org/99033003/diff/200001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:181:
Log.e(TAG, "AcousticEchoCanceler.create failed");
return after this or the next line NPEs!

https://codereview.chromium.org/99033003/diff/200001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:185:
Log.e(TAG, "setEnabled error: " + ret);
return after this.

[Ami GONE FROM CHROMIUM, 2013-12-12 02:29:32]

LGTM for the java bits stands.

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
On 2013/12/12 02:01:20, Ami Fischman wrote:
> On 2013/12/12 01:51:27, ajm wrote:
> > On 2013/12/11 22:11:38, Ami Fischman wrote:
> > > Perhaps that something is a copy ctor for PlatformEffects ?
> > 
> > This would require an assignment operator, right? And we still can't assign
to
> > the const member.
> > 
> > It's changed to a bitmask now, so things are a bit different, but please let
> me
> > know if I'm misinterpreting, and there's some way to use assignment when the
> > type has a const member.
> 
> You're misinterpreting.
> "Copy constructor" is a term of art in C++ and is different to assignment
> operator.  Because the copy ctor is a ctor it gets to assign to const members.
> http://en.wikipedia.org/wiki/Copy_constructor
> 
> (I don't actually believe you don't know this; I'm just being explicit to make
> sure we're communicating :))

I was failing to process that default-assignment-operator of AudioParameters
(which is used in multiple places, apparently) precludes using the copy ctor
unless placement new is involved, which would be crazytalk.  Fine to keep
mutable, though please add a comment explaining it's only mutable to support
entire-struct (re-)assignment, not individual setting twiddling.

[ajm, 2013-12-12 03:36:02]

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/60001/media/audio/audio_paramet...
media/audio/audio_parameters.h:59: void Reset(Format format, ChannelLayout
channel_layout,
On 2013/12/12 02:29:33, Ami Fischman wrote:
> I was failing to process that default-assignment-operator of AudioParameters
> (which is used in multiple places, apparently) precludes using the copy ctor
> unless placement new is involved, which would be crazytalk.  Fine to keep
> mutable, though please add a comment explaining it's only mutable to support
> entire-struct (re-)assignment, not individual setting twiddling.

Done.

https://codereview.chromium.org/99033003/diff/200001/media/base/android/java/...
File media/base/android/java/src/org/chromium/media/AudioRecordInput.java
(right):

https://codereview.chromium.org/99033003/diff/200001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:181:
Log.e(TAG, "AcousticEchoCanceler.create failed");
On 2013/12/12 02:01:20, Ami Fischman wrote:
> return after this or the next line NPEs!

Arg, done.

https://codereview.chromium.org/99033003/diff/200001/media/base/android/java/...
media/base/android/java/src/org/chromium/media/AudioRecordInput.java:185:
Log.e(TAG, "setEnabled error: " + ret);
On 2013/12/12 02:01:20, Ami Fischman wrote:
> return after this.

Done.

https://codereview.chromium.org/99033003/diff/240001/media/audio/audio_parame...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/240001/media/audio/audio_parame...
media/audio/audio_parameters.h:112: // not be mutated individually.
Dale, Tommi: To legitimize this comment, I removed the SetDiscreteChannels()
method, which was only used in a single unittest file. Is that OK?

[Jói, 2013-12-12 11:05:20]

//content/public LGTM.

[tommi (sloooow) - chröme, 2013-12-12 12:05:31]

lgtm

https://codereview.chromium.org/99033003/diff/240001/media/audio/android/audi...
File media/audio/android/audio_android_unittest.cc (right):

https://codereview.chromium.org/99033003/diff/240001/media/audio/android/audi...
media/audio/android/audio_android_unittest.cc:101:
AudioParameters::ECHO_CANCELLER);
nit: it's not clear to me what indent reference is being used here. Maybe just
do:

<< "echo_canceller: "
<< (params.effects() & AudioParameters::ECHO_CANCELLER);

since you already need a new line?

https://codereview.chromium.org/99033003/diff/240001/media/audio/audio_parame...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/240001/media/audio/audio_parame...
media/audio/audio_parameters.h:112: // not be mutated individually.
On 2013/12/12 03:36:03, ajm wrote:
> Dale, Tommi: To legitimize this comment, I removed the SetDiscreteChannels()
> method, which was only used in a single unittest file. Is that OK?

sgtm.

Dale - afaict we never actually set layout to CHANNEL_LAYOUT_DISCRETE.  Should
we remove that enum value?

[tommi (sloooow) - chröme, 2013-12-12 12:07:07]

fwiw - I think it's OK to TBR the owners review for
renderer_webkitplatformsupport_impl.cc.

[ajm, 2013-12-12 17:35:04]

Sorry about the rebase in patch set 11. I've modified:
audio_android_unittest.cc
audio_manager_mac.cc
audio_manager_win.cc
AudioManagerAndroid.java
AudioRecordInput.java
channel_mixer_unittest.cc

https://codereview.chromium.org/99033003/diff/240001/media/audio/android/audi...
File media/audio/android/audio_android_unittest.cc (right):

https://codereview.chromium.org/99033003/diff/240001/media/audio/android/audi...
media/audio/android/audio_android_unittest.cc:101:
AudioParameters::ECHO_CANCELLER);
On 2013/12/12 12:05:31, tommi wrote:
> nit: it's not clear to me what indent reference is being used here. Maybe just
> do:
> 
> << "echo_canceller: "
> << (params.effects() & AudioParameters::ECHO_CANCELLER);
> 
> since you already need a new line?

Done.

https://codereview.chromium.org/99033003/diff/240001/media/audio/audio_parame...
File media/audio/audio_parameters.h (right):

https://codereview.chromium.org/99033003/diff/240001/media/audio/audio_parame...
media/audio/audio_parameters.h:112: // not be mutated individually.
On 2013/12/12 12:05:31, tommi wrote:
> On 2013/12/12 03:36:03, ajm wrote:
> > Dale, Tommi: To legitimize this comment, I removed the SetDiscreteChannels()
> > method, which was only used in a single unittest file. Is that OK?
> 
> sgtm.
> 
> Dale - afaict we never actually set layout to CHANNEL_LAYOUT_DISCRETE.  Should
> we remove that enum value?

It was actually used in audio_manager_mac.cc as well; codesearch cross-reference
didn't catch that oddly (perhaps it only evaluates the calls on Linux?). Updated
now.

[tommi (sloooow) - chröme, 2013-12-12 17:46:00]

Lgtm. Looking forward to hearing this is action!
On Dec 12, 2013 6:35 PM, <ajm@chromium.org> wrote:

> Sorry about the rebase in patch set 11. I've modified:
> audio_android_unittest.cc
> audio_manager_mac.cc
> audio_manager_win.cc
> AudioManagerAndroid.java
> AudioRecordInput.java
> channel_mixer_unittest.cc
>
>
> https://codereview.chromium.org/99033003/diff/240001/
> media/audio/android/audio_android_unittest.cc
> File media/audio/android/audio_android_unittest.cc (right):
>
> https://codereview.chromium.org/99033003/diff/240001/
> media/audio/android/audio_android_unittest.cc#newcode101
> media/audio/android/audio_android_unittest.cc:101:
> AudioParameters::ECHO_CANCELLER);
> On 2013/12/12 12:05:31, tommi wrote:
>
>> nit: it's not clear to me what indent reference is being used here.
>>
> Maybe just
>
>> do:
>>
>
>  << "echo_canceller: "
>> << (params.effects() & AudioParameters::ECHO_CANCELLER);
>>
>
>  since you already need a new line?
>>
>
> Done.
>
> https://codereview.chromium.org/99033003/diff/240001/
> media/audio/audio_parameters.h
> File media/audio/audio_parameters.h (right):
>
> https://codereview.chromium.org/99033003/diff/240001/
> media/audio/audio_parameters.h#newcode112
> media/audio/audio_parameters.h:112: // not be mutated individually.
> On 2013/12/12 12:05:31, tommi wrote:
>
>> On 2013/12/12 03:36:03, ajm wrote:
>> > Dale, Tommi: To legitimize this comment, I removed the
>>
> SetDiscreteChannels()
>
>> > method, which was only used in a single unittest file. Is that OK?
>>
>
>  sgtm.
>>
>
>  Dale - afaict we never actually set layout to CHANNEL_LAYOUT_DISCRETE.
>>
>  Should
>
>> we remove that enum value?
>>
>
> It was actually used in audio_manager_mac.cc as well; codesearch
> cross-reference didn't catch that oddly (perhaps it only evaluates the
> calls on Linux?). Updated now.
>
> https://codereview.chromium.org/99033003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ajm, 2013-12-12 17:46:12]

Justin, could you have a look at media_stream_messages.h for owner security
approval?

[jamesr, 2013-12-12 17:56:28]

lgtm

[DaleCurtis, 2013-12-12 20:30:15]

AudioParameters lgtm

[commit-bot: I haz the power, 2013-12-12 22:44:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajm@chromium.org/99033003/320001

[ajm, 2013-12-12 22:50:54]

Justin, I TBR'd this to you as the change is trivial, but please let me know if
there are any concerns.

[jschuh, 2013-12-12 23:10:47]

ipc security lgtm: simple bitmask

[commit-bot: I haz the power, 2013-12-13 05:12:05]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-13 05:46:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ajm@chromium.org/99033003/320001

[commit-bot: I haz the power, 2013-12-13 08:39:43]

Change committed as 240548

[no longer working on chromium, 2013-12-30 14:55:55]

driven-by, I know this CL works with the use cases that you specified in the
description, but there are some problems that might introduce damages for the
future.

https://codereview.chromium.org/99033003/diff/320001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/99033003/diff/320001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:382:
CreateLocalAudioSource(&constraints).get());
you modify the constraints and save the change in the audio options owned by the
LocalAudioSource, but the change is not known by other classes which use the
constraints, for example, WebRtcLocalAudioTrack.

https://codereview.chromium.org/99033003/diff/320001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:745:
source_data->SetLocalAudioSource(CreateLocalAudioSource(constraints).get());
shouldn't you do the same thing for the case when using webaudio ?

[ajm, 2014-01-08 01:35:01]

Thanks for having a look Shijing.

https://codereview.chromium.org/99033003/diff/320001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/99033003/diff/320001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:382:
CreateLocalAudioSource(&constraints).get());
On 2013/12/30 14:55:56, xians1 wrote:
> you modify the constraints and save the change in the audio options owned by
the
> LocalAudioSource, but the change is not known by other classes which use the
> constraints, for example, WebRtcLocalAudioTrack.

We only need to update the constraints for paths which will be processing the
input audio. Does WebRtcLocalAudioTrack (or any other path) do that?

https://codereview.chromium.org/99033003/diff/320001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:745:
source_data->SetLocalAudioSource(CreateLocalAudioSource(constraints).get());
On 2013/12/30 14:55:56, xians1 wrote:
> shouldn't you do the same thing for the case when using webaudio ?

Agreed, I believe we should update the constraints here as well. Tommi, Henrik,
can you confirm?

[henrika (OOO until Aug 14), 2014-01-08 08:11:08]

> Agreed, I believe we should update the constraints here as well. Tommi,
Henrik,
> can you confirm?

Not my area of expertise; defer question to Tommi.

[tommi (sloooow) - chröme, 2014-01-09 15:33:15]

https://codereview.chromium.org/99033003/diff/320001/content/renderer/media/m...
File content/renderer/media/media_stream_dependency_factory.cc (right):

https://codereview.chromium.org/99033003/diff/320001/content/renderer/media/m...
content/renderer/media/media_stream_dependency_factory.cc:745:
source_data->SetLocalAudioSource(CreateLocalAudioSource(constraints).get());
On 2014/01/08 01:35:02, ajm wrote:
> On 2013/12/30 14:55:56, xians1 wrote:
> > shouldn't you do the same thing for the case when using webaudio ?
> 
> Agreed, I believe we should update the constraints here as well. Tommi,
Henrik,
> can you confirm? 

Yes.  Ideally the constraints should be consistent all the way up to blink.  At
the moment though this is pretty broken but "harmless" since the constraints are
currently only checked at construction.  This will change though in the near
future, so good to be aware of.