Re-land r9378 "Rename APM Config DelayCorrection to ExtendedFilter"

Re-land r9378 "Rename APM Config DelayCorrection to ExtendedFilter"

(This reverts commit 3fbf3f8841b5460503fb646eaedcb063620434a8.)

The original submission was reverted because it broke the Chrome build. This is fixed in patch set 2 of this change by keeping the old MediaConstraintsInterface string kExperimentalEchoCancellation. It will be removed once the Chrome code has been updated.

Original description:
"We use this Config struct for enabling/disabling Extended filter mode in AEC. This change renames it to ExtendedFilter for readability reasons. The corresponding media constraint is also renamed to kExtendedFilterEchoCancellation.

The old Config is kept in parallel with the new during a transition period. This is to avoid problems with API breakages. During this period, if any of the two Configs are enabled, the extended filter mode is engaged in APM. That is, the two Configs are combined with an "OR" operation.

This change also renames experimental_aec in AudioOptions to extended_filter_aec."

BUG=webrtc:4696
R=bjornv@webrtc.org, tommi@webrtc.org

Committed: https://chromium.googlesource.com/external/webrtc/+/441f6347311bcf2079435c3888d67e1fb321f9f8

[hlundin-webrtc, 2015-06-05 13:33:39]

New changes

[hlundin-webrtc, 2015-06-05 13:40:25]

henrik.lundin@webrtc.org changed reviewers:
+ bjornv@webrtc.org, tommi@webrtc.org

[hlundin-webrtc, 2015-06-05 13:40:25]

henrik.lundin@webrtc.org changed required reviewers:
+ bjornv@webrtc.org, tommi@webrtc.org

[hlundin-webrtc, 2015-06-05 13:45:33]

Björn, Tommi,

Please, re-review this CL. Patch set 1 is identical to the initial submission
which was reverted. Patch set 2 is the new changes that will fix the problem.

Thanks!

[bjornv1, 2015-06-05 13:59:04]

lgtm % nit

https://codereview.webrtc.org/1151573021/diff/20001/talk/app/webrtc/localaudi...
File talk/app/webrtc/localaudiosource.cc (right):

https://codereview.webrtc.org/1151573021/diff/20001/talk/app/webrtc/localaudi...
talk/app/webrtc/localaudiosource.cc:63:
MediaConstraintsInterface::kExtendedFilterEchoCancellation)
How about a comment/TODO to why you do this, even if it's supposed to be there
for a short time.

[hlundin-webrtc, 2015-06-08 11:35:43]

Adding a comment

[hlundin-webrtc, 2015-06-08 11:36:33]

Thanks Björn.

Ping Tommi.

https://codereview.webrtc.org/1151573021/diff/20001/talk/app/webrtc/localaudi...
File talk/app/webrtc/localaudiosource.cc (right):

https://codereview.webrtc.org/1151573021/diff/20001/talk/app/webrtc/localaudi...
talk/app/webrtc/localaudiosource.cc:63:
MediaConstraintsInterface::kExtendedFilterEchoCancellation)
On 2015/06/05 13:59:04, bjornv1 wrote:
> How about a comment/TODO to why you do this, even if it's supposed to be there
> for a short time.

Done.

[tommi, 2015-06-08 16:41:34]

one request...

https://codereview.webrtc.org/1151573021/diff/40001/talk/app/webrtc/localaudi...
File talk/app/webrtc/localaudiosource.cc (right):

https://codereview.webrtc.org/1151573021/diff/40001/talk/app/webrtc/localaudi...
talk/app/webrtc/localaudiosource.cc:63:
MediaConstraintsInterface::kExtendedFilterEchoCancellation)
hmm... don't suppose you can start adding braces :-/  as is (and before) this
goes against the style guide.
Alternatively, you could do something like this:

struct {
  const char* name;
  Settable<bool>& value;
} key_to_value[] = {
  { MediaConstraintsInterface::kEchoCancellation, options->echo_cancellation },
  // Both kExperimentalEchoCancellation (old) and
  // kExtendedFilterEchoCancellation (new) translate to extended_filter_aec
  // option being set. This is to manage the transition from the old to the
  // new without breaking dependent code.
  { MediaConstraintsInterface::kExperimentalEchoCancellation,
    options->extended_filter_aec },
  { MediaConstraintsInterface::kExtendedFilterEchoCancellation,
    options->extended_filter_aec },
  { MediaConstraintsInterface::kDAEchoCancellation,
    options->options->delay_agnostic_aec },
  ...etc
};

for (auto& entry : key_to_value) {
  if (iter->key.compare(entry.name) == 0)
    entry.value.Set(value);
}

I think it would also produce smaller code.

[hlundin-webrtc, 2015-06-09 08:42:51]

Tommi, PTAL again.

https://codereview.webrtc.org/1151573021/diff/40001/talk/app/webrtc/localaudi...
File talk/app/webrtc/localaudiosource.cc (right):

https://codereview.webrtc.org/1151573021/diff/40001/talk/app/webrtc/localaudi...
talk/app/webrtc/localaudiosource.cc:63:
MediaConstraintsInterface::kExtendedFilterEchoCancellation)
On 2015/06/08 16:41:34, tommi (webrtc) wrote:
> hmm... don't suppose you can start adding braces :-/  as is (and before) this
> goes against the style guide.
> Alternatively, you could do something like this:
> 
> struct {
>   const char* name;
>   Settable<bool>& value;
> } key_to_value[] = {
>   { MediaConstraintsInterface::kEchoCancellation, options->echo_cancellation
},
>   // Both kExperimentalEchoCancellation (old) and
>   // kExtendedFilterEchoCancellation (new) translate to extended_filter_aec
>   // option being set. This is to manage the transition from the old to the
>   // new without breaking dependent code.
>   { MediaConstraintsInterface::kExperimentalEchoCancellation,
>     options->extended_filter_aec },
>   { MediaConstraintsInterface::kExtendedFilterEchoCancellation,
>     options->extended_filter_aec },
>   { MediaConstraintsInterface::kDAEchoCancellation,
>     options->options->delay_agnostic_aec },
>   ...etc
> };
> 
> for (auto& entry : key_to_value) {
>   if (iter->key.compare(entry.name) == 0)
>     entry.value.Set(value);
> }
> 
> I think it would also produce smaller code.

Like that?

[tommi, 2015-06-09 08:50:12]

exactly :) Thanks.  lgtm % couple of minor things

https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
File talk/app/webrtc/localaudiosource.cc (right):

https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
talk/app/webrtc/localaudiosource.cc:59: } key_to_value[] = {
You could move this outside the loop (e.g. top of function)

https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
talk/app/webrtc/localaudiosource.cc:86: {MediaConstraintsInterface::kAecDump,
options->aec_dump}};
nit: Move }; to the next line

[hlundin-webrtc, 2015-06-09 09:01:05]

Moving the struct

[hlundin-webrtc, 2015-06-09 09:02:09]

Thanks! I will commit this version.

https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
File talk/app/webrtc/localaudiosource.cc (right):

https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
talk/app/webrtc/localaudiosource.cc:59: } key_to_value[] = {
On 2015/06/09 08:50:11, tommi (webrtc) wrote:
> You could move this outside the loop (e.g. top of function)

Done.

https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
talk/app/webrtc/localaudiosource.cc:86: {MediaConstraintsInterface::kAecDump,
options->aec_dump}};
On 2015/06/09 08:50:11, tommi (webrtc) wrote:
> nit: Move }; to the next line

Nope. This is clang format. I'm not fighting that windmill...

[tommi, 2015-06-09 09:50:15]

On 2015/06/09 09:02:09, hlundin-webrtc wrote:
> Thanks! I will commit this version.
> 
>
https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
> File talk/app/webrtc/localaudiosource.cc (right):
> 
>
https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
> talk/app/webrtc/localaudiosource.cc:59: } key_to_value[] = {
> On 2015/06/09 08:50:11, tommi (webrtc) wrote:
> > You could move this outside the loop (e.g. top of function)
> 
> Done.
> 
>
https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
> talk/app/webrtc/localaudiosource.cc:86: {MediaConstraintsInterface::kAecDump,
> options->aec_dump}};
> On 2015/06/09 08:50:11, tommi (webrtc) wrote:
> > nit: Move }; to the next line
> 
> Nope. This is clang format. I'm not fighting that windmill...

Note that clang format is a suggestion, not a rule.  Clang formatting has also
changed many times and into the direction of what the Chromium code has
traditionally looked like.  This is fine as is, but I wouldn't consider clang >
reviewer.

[hlundin-webrtc, 2015-06-09 13:58:11]

New line

[hlundin-webrtc, 2015-06-09 14:00:18]

https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
File talk/app/webrtc/localaudiosource.cc (right):

https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
talk/app/webrtc/localaudiosource.cc:86: {MediaConstraintsInterface::kAecDump,
options->aec_dump}};
On 2015/06/09 09:02:09, hlundin-webrtc wrote:
> On 2015/06/09 08:50:11, tommi (webrtc) wrote:
> > nit: Move }; to the next line
> 
> Nope. This is clang format. I'm not fighting that windmill...

Ok, ok. I picked a fight with the windmill anyway...

[hlundin-webrtc, 2015-06-09 14:03:30]

Committed patchset #6 (id:100001) manually as
441f6347311bcf2079435c3888d67e1fb321f9f8 (presubmit successful).

[kwiberg-webrtc, 2015-06-09 14:16:37]

https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
File talk/app/webrtc/localaudiosource.cc (right):

https://codereview.webrtc.org/1151573021/diff/60001/talk/app/webrtc/localaudi...
talk/app/webrtc/localaudiosource.cc:86: {MediaConstraintsInterface::kAecDump,
options->aec_dump}};
On 2015/06/09 at 14:00:18, hlundin-webrtc wrote:
> Ok, ok. I picked a fight with the windmill anyway...

Don't worry---I'm sure it'll be a breeze!